On Sun, Oct 10, 2010 at 11:28 PM, Eric Sunshine <ericsunshine@xxxxxxxxx> wrote: > On 10/10/2010 4:37 PM, Erik Faye-Lund wrote: >> >> 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. > > 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()? > 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<--- >> 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... -- 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