On Sun, Oct 10, 2010 at 9:50 PM, Eric Sunshine <ericsunshine@xxxxxxxxx> wrote:
> On 10/10/2010 9:20 AM, Erik Faye-Lund wrote:
>>
>> From: Mike Pape<dotzenlabs@xxxxxxxxx>
>>
>> Syslog does not usually exist on Windows, so we implement our own
>> using Window's ReportEvent mechanism.
>>
>> Signed-off-by: Mike Pape<dotzenlabs@xxxxxxxxx>
>> Signed-off-by: Erik Faye-Lund<kusmabite@xxxxxxxxx>
>> ---
>> +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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html