Add xsyslog_ev function and related #5186
Conversation
Also, more comments The history of these two commits is complicated: * MJD wrote an initial draft * Ken revised it to use struct buf instead of C strings * MJD came back and restored his original comments and replaced the printf hackery with smaller printf hackery NB: The diff for this commit is a mess --- *unless* you use Git's --patience option.
mjdominus-cyrus
force-pushed
the
cyr-1407-xsyslog_ev-1223
branch
from
c471a74 to
e5a4647
Compare
There was a problem hiding this comment.
I like improving the log format! It's also great we got proper unit tests for this.
Apart from the few items I found in the code, I wonder:
Is there a canonical specification for logfmt somewhere? I see our tests don't assert how empty strings in the key or value are handled, would that be the empty string or ""?
I'm a bit wary of the magic printf formatter guessing, but it looks well thought out so let's see how it turns out in practice.
One other thing that stands out to me are the commit message: in Cyrus, we typically prefix the commit message with the filename that mostly got changed in this commit (e.g. util.c). Also, our commit messages almost always start with a verb.
| // in place of | ||
| // xsyslog_ev(..., key, "foo", DUMMY, ...) | ||
| // but that seems morelikely to be confusing than otherwise | ||
| case -1: |
There was a problem hiding this comment.
This does not compile on my machine which runs on aarch64:
2360 // but that seems morelikely to be confusing than otherwise
E 2361 case -1: <-- error: Overflow converting case value to switch condition type (-1 to 255)
2362 default:
The reason for that is that on aarch64 the char type is unsigned. Luckily, in this case the fix just looks to be to remove that case -1 altogether, as it would fall through to the default label anyway.
There's also a nit in the line above: split "morelikely" into "more likely"
There was a problem hiding this comment.
I can do this. Would it also work to declare the function as returning signed char?
| while (*s && *s != '%') s++; // seek to first % sign or to end of string | ||
| if (*s == 0) return 0; // String has no percent signs at all | ||
| s++; | ||
| if (*s == '%') { s++; goto TOP; } // it's "%%", skip it and continue |
There was a problem hiding this comment.
nit: I know you asked before writing this code about when to and when to not use goto, so I won't request to change this now. But I find this goto here not only unnecessary but confusing. A while loop with a break or continue statement would make that code likely more simple to read.
As a rule of thumb: goto in the Cyrus codebase is perfectly fine for early function exits that require cleanup (e.g. the done label). In all other cases, they are the least preferred option.
|
@rsto I thought we'd improved the clarity of this in the Fastmail internal spec, but I'm wrong. We just use Perl's Log::Fmt, which I wrote. In that, the value must have length, so |
Per https://linear.app/fastmail/issue/CYR-1407/produce-new-xsyslog-ev-function
Also:
util.testcvalgrindis clean