From: Erik Faye-Lund Date: Sun, 10 Oct 2010 20:37:26 GMT Subject: Re: [msysGit] [PATCH v3 02/14] mingw: implement syslog Message-ID: In-Reply-To: <4CB2190F.6000908@gmail.com> On Sun, Oct 10, 2010 at 9:50 PM, Eric Sunshine wrote: > On 10/10/2010 9:20 AM, Erik Faye-Lund wrote: >> >> From: Mike Pape >> >> Syslog does not usually exist on Windows, so we implement our own >> using Window's ReportEvent mechanism. >> >> Signed-off-by: Mike Pape >> Signed-off-by: Erik Faye-Lund >> --- >> +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... >> +       /* >> +        * 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