Re: [msysGit] [PATCH v3 02/14] mingw: implement syslog
- From
- Eric Sunshine <ericsunshine@gmail.com>
- Date
- Oct 10, 2010, 21:28 UTC
- Message-ID
- <4CB22FF3.5070503@gmail.com>
- In-Reply-To
- <AANLkTinsqAOj7LtACpbcOrVZfeUApDjmQe2uYLH8npBF@mail.gmail.com>
On 10/10/2010 4:37 PM, Erik Faye-Lund wrote:
Show 31 quoted lines
> 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()?
Show 31 quoted lines
>>> + /* >>> + * 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