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

On 10/10/2010 4:37 PM, Erik Faye-Lund wrote:

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

quoted

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.

In retrospect, when thinking more carefully about the conditional 
expression, I suppose the code is self-documenting, though perhaps a 
comment in code or in commit message would help.

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.

Do you mean vsnprintf() rather than va_copy()?

quoted

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

I am not suggesting reporting an error. As a first-time reader of the 
code, I was trying to understand the presence of the comment which did 
not really seem to relate to the code. Perhaps adding a "FIXME" to the 
comment saying that the condition should perhaps be handled in the 
future would help to explain the comments presence.

(On the other hand, for the '%s' check above, the code does report a 
warning and then exits, so it is not inconceivable that a '%n' could 
also emit a warning.)

-- ES