Hi Tristan,
On Tue, Oct 17, 2023 at 06:19:57PM +0000, Tristan wrote:
> By default, messages printed from LUA log functions are sent both to
> the configured log target and additionally to stderr (in most cases).
> This introduces tune.lua.also-log-to-stderr for disabling that
> second copy of the message being sent to stderr.
>
> Addresses https://github.com/haproxy/haproxy/issues/2316
>
> This could be backported if wanted, since it preserves the behaviour
> that existed prior to it.
I agree with this, it has already annoyed me a few times in the past
when trying to debug something else. I have vague memories of the Lua
integration having been done with lots of fprintf() for debugging and
that finally it was cleaned up and considered good enough for a first
version. Since it was quite new by then, feedback and debugging were
expected so it was not a problem. But it has ossified into a default
behavior which is not longer desirable in my opinion.
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".
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 ;-)
- should we really stick to "on" as the default behavior in 2.9 ? I
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 ?
A few comments on the patch:
> diff --git a/src/hlua.c b/src/hlua.c
> index c686f222a..261aee763 100644
> --- a/src/hlua.c
> +++ b/src/hlua.c
> @@ -69,6 +69,12 @@
> #include <haproxy/check.h>
> #include <haproxy/mailers.h>
>
> +/* Global LUA on/off flags */
> +/* if on, LUA-originating logs are duplicated to stderr */
> +#define HLUA_TUNE_ALSO_LOG_TO_STDERR (1<<0)
Please leave several spaces between the name and the value so that other
names do not require to realign everything. For settings, it tends to be
more convenient to use hex values (padded left, e.g. 0x00000001) because
it allows masks and combined values to be represented as well in a visual
way, something that's basically unreadable when dealing with shifts. If
we implement "off", "auto", "on" here it will already be the case.
> +
> +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).
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.
> +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.
Please have a look at these points (or comment if you disagree), and
I'll happily merge it!
Thanks,
Willy
