Hi Tristan,
On Fri, Oct 20, 2023 at 04:19:34PM +0000, Tristan wrote:
> Hi all again,
>
> Here is the updated patch set after changes based on feedback received.
Thanks for doing this work.
> The change is now split across 2 patches.
>
> Patch 0001 adding:
> - tune.lua.log { on | off } (defaults to 'on') for usage of loggers
> - tune.lua.log-stderr { on | auto | off } (defaults to 'on') for usage of
> stderr
I'm still finding these a bit confusing, due to the naming. It seems to
me that the first one is sufficient to disable everything including the
second (due to the name only, given that you explained it's not the case).
I got your point regarding my perception of "alerts" vs "logs". I thought
these were used by alerts and in fact I noticed from your patch on the doc
that it applies to txn.log(), core.log() and so on, so that's definitely
related to logging.
Just an idea, not necessarily the best one, but maybe "tune.lua.log"
should be called "tune.lua.log-loggers" instead ? otherwise we can go
back to a single multi-combinations option (but this leaves less room
for later extensions if other ways ever appear such as rings or anything
else).
> Effectively no change to current behaviour, hence fit for backport.
>
> Patch 0002 changing:
> - tune.lua.log-stderr default from 'on' to 'auto'
>
> Changes behaviour, hence no backport.
>
> I guess my only remaining question is whether we want regtests for this?
>
> I'm be happy to write some if yes, but I know regtests runtime is a concern
> and it'd require a bit of test infra work to make capturing stderr output,
> so I just want to be sure it is desirable before :-)
I'd say it's as you want. We can't test stderr in regtests so the test
will be limited anyway. Also I don't expect such a thing to be broken
by accident, which is the first purpose of regtests.
> diff --git a/src/hlua.c b/src/hlua.c
> index e94325727..70a25e762 100644
> --- a/src/hlua.c
> +++ b/src/hlua.c
> @@ -69,6 +69,20 @@
> #include <haproxy/check.h>
> #include <haproxy/mailers.h>
>
> +/* Global LUA flags */
> +/* log handling */
> +#define HLUA_TUNE_LOG 0x00000001 /* send to current logger */
> +#define HLUA_TUNE_LOG_STDERR_ON 0x00000010 /* send to stderr */
> +#define HLUA_TUNE_LOG_STDERR_AUTO 0x00000020 /* send to stderr if no logger
> in use */
> +#define HLUA_TUNE_LOG_STDERR_OFF 0x00000040 /* never send to stderr */
> +#define HLUA_TUNE_LOG_STDERR_MASK 0x00000070
Please do not add flags for "off", and in general, flags are supposed to
be cumulable. By this I mean that above you could have ON|OFF|AUTO, which
will not make much sense in terms of configuration. The right approach
when you deal with multiple values like this for a same setting is to
group them within a bit mask as an enum. I also suggest that the OFF
value is the zero one, it simplifies lots of tests and more importantly
makes the code look more intuitive. Thus you would have for example:
#define HLUA_TUNE_LOG_STDERR_OFF 0x00000000 /* never send to stderr */
#define HLUA_TUNE_LOG_STDERR_ON 0x00000010 /* send to stderr */
#define HLUA_TUNE_LOG_STDERR_AUTO 0x00000020 /* send to stderr if no logger
in use */
#define HLUA_TUNE_LOG_STDERR_MASK 0x00000030 /* mask to retrieve stderr
logging */
Then you'll read a value like this:
(hlua_tune_flags & HLUA_TUNE_LOG_STDERR_MASK) == HLUA_TUNE_LOG_STDERR_xxx
In order to set such a value when parsing the config, you can just do:
hlua_tune_flags &= ~HLUA_TUNE_LOG_STDERR_MASK;
hlua_tune_flags |= HLUA_TUNE_LOG_STDERR_xxx;
And I'm seeing later in your patch that it's exactly what you did for
these, which is fine.
Also please do not leave holes in bit fields; I know it's tempting to
leave room for later extensions but in the end it leaves more smaller
holes which are harder to fill, so you can safely regroup them on bits
1 and 2 (thus values 0,2,4,6).
> +/* default flag values */
> +#define HLUA_TUNE_FLAGS_DFLT 0x00000011
This one can be quite a trap: if flags are later reordered, you can be
sure this one will not be updated. Better set the value based on the
field names.
> +/* flags made of HLUA_TUNE_* */
> +static uint hlua_tune_flags = HLUA_TUNE_FLAGS_DFLT;
> +
> /* Lua uses longjmp to perform yield or throwing errors. This
> * macro is used only for identifying the function that can
> * not return because a longjmp is executed.
> @@ -1366,8 +1380,9 @@ const char *hlua_show_current_location(const char *pfx)
> return NULL;
> }
>
> -/* This function is used to send logs. It try to send on screen (stderr)
> - * and on the default syslog server.
> +/* This function is used to send logs. It tries to send them to:
> + * - the log target applicable in the current context, AND
> + * - stderr if not in quiet mode or explicitly disabled
> */
> static inline void hlua_sendlog(struct proxy *px, int level, const char *msg)
> {
> @@ -1392,8 +1407,28 @@ static inline void hlua_sendlog(struct proxy *px, int
> level, const char *msg)
> }
> *p = '\0';
>
> - send_log(px, level, "%s\n", trash.area);
> + if (hlua_tune_flags & HLUA_TUNE_LOG)
> + send_log(px, level, "%s\n", trash.area);
> +
> if (!(global.mode & MODE_QUIET) || (global.mode & (MODE_VERBOSE |
> MODE_STARTING))) {
> + if (hlua_tune_flags & HLUA_TUNE_LOG_STDERR_OFF)
> + return;
> +
> + /* when logging via stderr is set to 'auto', it behaves like
> 'off' unless one of:
> + * - logging via loggers is disabled
> + * - this is a non-proxy context and there is no global logger
> configured
> + * - this is a proxy context and the proxy has no logger
> configured
> + */
> + if ((hlua_tune_flags & HLUA_TUNE_LOG_STDERR_AUTO) &&
> (hlua_tune_flags & HLUA_TUNE_LOG)) {
The simplest way to test for a combination of values within the same
field is to apply both masks and compare to the values you expect.
With the mask proposed above that would give:
if ((hlua_tune_flags & (HLUA_TUNE_LOG_STDERR_MASK |
HLUA_TUNE_LOG)) ==
(HLUA_TUNE_LOG_STDERR_AUTO | HLUA_TUNE_LOG)) {
See ? it checks for the exact combination you're interested in,
ie both AUTO and LOG. It's particularly convenient when checking
for inverted options (e.g. auto and !log) since it makes the
combinations explicit and avoids the risk of missing a "!" in
front of a parenthesis during a refactoring for example.
> + /* AUTO=OFF in non-proxy context only if at least one
> global logger is defined */
> + if ((px == NULL) && (!LIST_ISEMPTY(&global.loggers)))
> + return;
> +
> + /* AUTO=OFF in proxy context only if at least one
> logger is configured for the proxy */
> + if ((px != NULL) && (!LIST_ISEMPTY(&px->loggers)))
> + return;
> + }
> +
> if (level == LOG_DEBUG && !(global.mode & MODE_DEBUG))
> return;
(...)
No more comments, overall this looks good to me. Thus in summary, let's
try to avoid the ambiguity of "tune.lua.log" alone, and re-adjust the
bitfields. By the way, if we're having the same prefix "tune.lua.log"
for both options, it's an indication that we should likely have a dot
after to enable loggers (or any other name) and stderr:
tune.lua.log.loggers on|off
tune.lua.log.stderr on|auto|off
and we can get rid of the dash in the middle that probably someone in a
few years will question us about and we'll respond "it's historic" :-)
Thanks!
Willy
