From: Eric Sunshine <ericsunshine@gmail.com>
Date: Sun, 10 Oct 2010 21:28:19 GMT
Subject: Re: [msysGit] [PATCH v3 02/14] mingw: implement syslog
Message-ID: <4CB22FF3.5070503@gmail.com>
In-Reply-To: <AANLkTinsqAOj7LtACpbcOrVZfeUApDjmQe2uYLH8npBF@mail.gmail.com>

On 10/10/2010 4:37 PM, Erik Faye-Lund wrote:
> On Sun, Oct 10, 2010 at 9:50 PM, Eric Sunshine<ericsunshine@gmail.com>  wrote:
>> On 10/10/2010 9:20 AM, Erik Faye-Lund wrote:
>>>
>>> From: Mike Pape<dotzenlabs@gmail.com>
>>>
>>> Syslog does not usually exist on Windows, so we implement our own
>>> using Window's ReportEvent mechanism.
>>>
>>> Signed-off-by: Mike Pape<dotzenlabs@gmail.com>
>>> Signed-off-by: Erik Faye-Lund<kusmabite@gmail.com>
>>> ---
>>> +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()?

>>> +       /*
>>> +        * 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