Re: [msysGit] [PATCH/RFC 03/11] mingw: implement syslog

From: Johannes Sixt <j6t@kdbg.org>
Date: Nov 26, 2009, 21:23 UTC
Message-ID: <200911262223.22777.j6t@kdbg.org>
In-Reply-To: <1259196260-3064-4-git-send-email-kusmabite@gmail.com>

On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:

> +void syslog(int priority, const char *fmt, ...) {

Style: Brace goes to next line.

Show 8 quoted lines

> +	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);

Show 9 quoted lines

> +	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.

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?

-- Hannes