Hi Willy,
On 18/10/2023 07:47, Willy Tarreau wrote:
Hi Tristan,
...
I'm fine with the general approach, but I'm having two comments:
- using the word "also" in the option name is really not welcome
("tune.lua.also-log-to-stderr"), and actually more confusing than
without because one might wonder "why also, where is it also sent".
Funny you say that, because that's exactly what I intended by using
"also". Since what is surprising to me about this behavior is less that
it prints to stderr and more that it ADDITIONALLY prints to stderr.
Which is why I thought emphasizing it made sense.
Most of our options are compatible and cumulable, and not exclusive,
so it should be seen as a boolean like any other one. As such I
would just call it "tune.lua.log-stderr" and be done with it. That
may even allow you not to resize the keywords array, which could
help with backports ;-)
Not resizing the array is ideal ideed, but I'd argue that "log-stderr"
is a little bit misleading, since it's really not a switch between two
output streams, but rather a switch for a second one (stderr) being also
written to.
Though frankly I don't care THAT much so if everyone's fine with it, I
also am.
For the record, I agree it is ugly wording though, but I looked for what
similar flags existed and how other software defined them, and
"alsologtostderr" seems to be the common approach these days (though imo
such a flag shouldn't even exist in the first place).
- should we really stick to "on" as the default behavior in 2.9 ? I
I'd be up for that, but I expected to be told off. If no one objects I'm
more than happy to make it default. Or rather call it
"tune.lua.log-stderr" as you propose, and default that one to "off".
sense that basically nobody wants that by default anymore, and if
we want to change the default, it will only be in an odd version,
hence 3.1 for the next one. Maybe now's the right moment ? Or if
the concern is to lose the messages when no logs are configured,
maybe we can have a 3rd value "auto" which would be the default
and which would only log to stderr if there's no other logger ? I
don't know if we have this info where it's used, though. Hmmm at
first glance we seem to have it by testing if either px->loggers
is non-empty or global.loggers is non-empty. Thus it could be the
nicest approach, having "on" by default in 2.8 and older and "auto"
on 2.9 maybe ?
I suppose that work too. I actually never ran HAProxy without any log
config and thought defaults would be equivalent to:
global
log stderr format raw local0
default
log global
(ie that at runtime logger fields were always set)
A few comments on the patch:
[...]
+static int hlua_tune_flags = HLUA_TUNE_ALSO_LOG_TO_STDERR;
When you're using such arrays of flags, please precede them with a short
comment saying "flags made of HLUA_TUNE_*" or something like this so that
if it ever grows and more stuff gets inserted in between, it remains easy
to figure (that's one issue that has affected all other ones over time).
Fair point. Feared it'd be a bit repetitive with the comment just above,
but I see what you mean.
To be honest I wasn't even sure where in the file I should put it. Or if
it was preferable to add it in global.tune.options.
But I saw above that struct a comment saying it needed to be redone
"properly" so I figured I shouldn't add to it.
Also for bit fields, I'd prefer to use an unsigned int (we have "uint"
as a short form) so that when you see them in gdb you don't get negative
values that are even more annoying to copy-paste and decode.
Ah, sure.
Thought so too, but guessed that there was a reason for signed ints
since global.tune.options uses that.
+static int hlua_also_log_to_stderr(char **args, int section_type, struct proxy
*curpx,
+ const struct proxy *defpx, const char
*file, int line,
+ char **err)
It's not obvious at all that this function is there to parse a keyword,
I'm seeing it as the one which will actually log. Almost all of our
keyword parsing functions (others historically have "check"). I'm
seeing that it's not the case for the majority of the historic Lua
functions, but this should not be a reason for not fixing it (and
maxmem was fixed already). Better call it "hlua_parse_log_stderr" or
something like this that reminds the keyword.
Got it. Definitely agree, and only went that route for consistency at
first 👍
Please have a look at these points (or comment if you disagree), and
I'll happily merge it!
I'll follow up with the requested changes, however I'm not familiar with
patches over mailing lists. Should the updated patch be sent in its own
thread or as a reply of this with the diff file in attachment?
Tristan
