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()?
+ /*
+ * 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
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.
(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.)
-- ES
--
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