Re: [msysGit] [PATCH v3 02/14] mingw: implement syslog

On Sun, Oct 10, 2010 at 11:28 PM, Eric Sunshine [off-list ref] wrote:

On 10/10/2010 4:37 PM, Erik Faye-Lund wrote:

quoted

On Sun, Oct 10, 2010 at 9:50 PM, Eric Sunshine[off-list ref]
 wrote:

quoted

On 10/10/2010 9:20 AM, Erik Faye-Lund wrote:

quoted

From: Mike Pape<redacted>

Syslog does not usually exist on Windows, so we implement our own
using Window's ReportEvent mechanism.

Signed-off-by: Mike Pape<redacted>
Signed-off-by: Erik Faye-Lund<redacted>
---
+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.

quoted

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()?

OK, I had to read some old discussions to figure out what the issue was :)

The problem was lack of portable va_copy, because I tried to add a
non-variadic version of strbuf_addf(), namely strbuf_vaddf() to do the
work.

I guess it could be implemented pretty easily with vsnprintf(),
though. I was afraid of doing that originally because I know there's
portability issues with the return value of snprintf. Luckily it seems
that we have a fix for that in compat/sprintf.c, and we rely on the
return value being correct in strbuf_addf() so it would probably be
safe.

Something like this (on top)

---8<---

diff --git a/compat/mingw.c b/compat/mingw.c
index bbe45d0..e3f3f92 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c

@@ -1435,17 +1435,24 @@ void openlog(const char *ident, int logopt,

int facility)
 		warning("RegisterEventSource() failed: %lu", GetLastError());
 }

-void syslog(int priority, const char *fmt, const char *arg)
+void syslog(int priority, const char *fmt, ...)
 {
 	WORD logtype;
+	char *str;
+	int str_len;
+	va_list ap;

 	if (!ms_eventlog)
 		return;

-	if (strcmp(fmt, "%s")) {
-		warning("format string of syslog() not implemented");
-		return;
-	}
+	va_start(ap, fmt);
+	str_len = vsnprintf(NULL, 0, fmt, ap);
+	va_end(ap);
+
+	str = malloc(str_len + 1);
+	va_start(ap, fmt);
+	vsnprintf(str, str_len, fmt, ap);
+	va_end(ap);

 	switch (priority) {
 	case LOG_EMERG:

@@ -1478,8 +1485,9 @@ void syslog(int priority, const char *fmt, const

char *arg)
 	    NULL,
 	    1,
 	    0,
-	    (const char **)&arg,
+	    (const char **)&str,
 	    NULL);
+	free(str);
 }

 #undef signal

diff --git a/compat/mingw.h b/compat/mingw.h
index aed49d8..45a63a0 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h

@@ -186,7 +186,7 @@ int setitimer(int type, struct itimerval *in,

struct itimerval *out);
 int sigaction(int sig, struct sigaction *in, struct sigaction *out);
 int link(const char *oldpath, const char *newpath);
 void openlog(const char *ident, int logopt, int facility);
-void syslog(int priority, const char *fmt, const char *arg);
+void syslog(int priority, const char *fmt, ...);

 /*
  * replacements of existing functions
---8<---

quoted

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.

A FIXME would certainly a good idea, if I don't just end up supporting
varargs here.

(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.)

I guess I could add something like this:

if (strstr(arg, "%1"))
	warning("arg contains %1, message might be corrupted");

I don't want to return in that case, because I think some output is
better than no output, and it seems to work on Vista. In fact, working
on Vista is kind of demotivating me to add such a warning in the first
place...