Re: [msysGit] [PATCH v3 02/14] mingw: implement syslog

On Sun, Oct 10, 2010 at 9:50 PM, Eric Sunshine [off-list ref] wrote:

On 10/10/2010 9:20 AM, Erik Faye-Lund wrote:

quoted

From: Mike Pape<redacted>

Syslog does not usually exist on Windows, so we implement our own
using Window's ReportEvent mechanism.

Signed-off-by: Mike Pape<redacted>
Signed-off-by: Erik Faye-Lund<redacted>
---
+void syslog(int priority, const char *fmt, const char *arg)
+{
+       WORD logtype;
+
+       if (!ms_eventlog)
+               return;
+
+       if (strcmp(fmt, "%s")) {
+               warning("format string of syslog() not implemented");
+               return;
+       }

It is not exactly clear what the intention is here. Is this trying to say
that no formatting directives are allowed in 'fmt' or what? The simple case
it is actually checking (where 'fmt' is solely '%s') could easily be handled
manually, as could more complex formats.

This is the result of the feed-back in v1, where we tried to implement
all format strings. But that turned out to be very complex (due to the
lack of a portable va_copy()) and since we control all call-sites for
syslog and already only use "%s" as the format, it should be OK.

Perhaps that should be mentioned in the commit message...

quoted

+       /*
+        * ReportEvent() doesn't handle strings containing %n, where n is
+        * an integer. Such events must be reformatted by the caller.
+        */
+       ReportEventA(ms_eventlog,
+           logtype,
+           0,
+           0,
+           NULL,
+           1,
+           0,
+           (const char **)&arg,
+           NULL);

The comment about '%n' seems to be warning about a potential problem but
does not actually protect against it. Should this issue be handled?

This is again an issue that was discussed in the first round.
ReportEvent() CANNOT report a string containing "%n" (where n is an
integer). And while we could probably try to work around it by
inserting a space or something, and I don't think we ever were able to
find a case where we could report a string containing "%n" in the
first place...

Are you suggesting that we report an error when we can't report the
string correctly? We could do that, but I'm not sure how the end-user
would benefit from that. ReportEvent is used to report errors (unless
the --verbose flag has been specified), and reporting that we can't
present an error message strike me as a bit confusing... Even the
corrupted error message is probably better :P