From: Erik Faye-Lund <kusmabite@googlemail.com>
Date: Fri, 27 Nov 2009 08:09:11 GMT
Subject: Re: [msysGit] [PATCH/RFC 03/11] mingw: implement syslog
Message-ID: <40aa078e0911270009u7569cfe5gb250092c8d2c0eac@mail.gmail.com>
In-Reply-To: <200911262223.22777.j6t@kdbg.org>

On Thu, Nov 26, 2009 at 10:23 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:
>> +     struct strbuf msg;
>> +     va_list va;
>> +     WORD logtype;
>> +
>> +     strbuf_init(&msg, 0);
>> +     va_start(va, fmt);
>> +     strbuf_vaddf(&msg, fmt, va);
>> +     va_end(va);
>
> I would
>
>        const char* arg;
>        if (strcmp(fmt, "%s"))
>                die("format string of syslog() not implemented")
>        va_start(va, fmt);
>        arg = va_arg(va, char*);
>        va_end(va);
>
> because we have exactly one invocation of syslog(), which passes "%s" as
> format string. Then strbuf_vaddf is not needed. Or even simpler: declare the
> function as
>
> void syslog(int priority, const char *fmt, const char*arg);

After reading this again, I agree that this is the best solution. I'll
update for the next iteration.

>> +     ReportEventA(ms_eventlog,
>> +         logtype,
>> +         (WORD)NULL,
>> +         (DWORD)NULL,
>> +         NULL,
>> +         1,
>> +         0,
>> +         (const char **)&msg.buf,
>> +         NULL);
>
> Why do you pass NULL and apply casts? The first one gives a warning.

I'll remove the casts for the next iteration.

>
> Did you read this paragraph in the documentation?
> http://msdn.microsoft.com/en-us/library/aa363679(VS.85).aspx
>
> "Note that the string that you log cannot contain %n, where n is an integer
> value (for example, %1) because the event viewer treats it as an insertion
> string. ..."
>
> How are the chances that this condition applies to our use of the function?
>

Ugh, increasingly high since we're adding IPv6 support, I guess.
Perhaps some form of escaping needs to be done?

-- 
Erik "kusma" Faye-Lund