On Sat, Feb 03, 2024 at 10:31:02AM +0000, Tristan wrote:
> Hi Willy,
>
> > On 3 Feb 2024, at 12:48, Willy Tarreau <w...@1wt.eu> wrote:
>
> > in fact we could check for
> >> the presence of "and" or "or" on a line, or some other suspicious
> >> constructs
>
> Another approach might be to optionally enforce quotes around strings.
>
> While it'd be a breaking change and sounds a bit difficult to offer as a
> flag, it avoids a lot of the current traps I think.
> Though I can imagine that many wouldn't like that (hence my pointing out to a
> flag or something).
Quite honestly, we've though about it several times but you can't enforce
such a change on 20 years of configs everywhere. We've already added support
for quotes by breaking some existing stuff in a very minimal way (there were
actually configs where a quote was meant to match a quote), and all we can
do at this point is encourage such practices but not much more. Also that
complicates concatenation and mixing of multiple modes (single/double quotes
for example). And after all, in shell, strings do not need to be quoted and
when this becomes a difficulty, users just learn to put quotes to visually
isolate their words.
> Essentially, the current state of things reminds me a bit of some of the
> error-prone-ness of YAML in that regard (implicit strings).
> Similarly I can never remember the cases where it's string, or "string", or
> str(string), or if quoting a string will maybe include the quotes in the
> actual value, and which ones are equivalent
That's exactly the same in shell, you never know how many backslashes you
have to insert inside quotes to put other quotes.
> > Well, this one was took just about the time needed for coffee to start to
> > make its effect :-)
> >
> > How about this:
> [...]
> >
> > It gives me no warning but triggers many diag warnings:
> >
> > $ ./haproxy -c -f diag.cfg
> > $ ./haproxy -dD -c -f diag.cfg
> > [NOTICE] (23315) : haproxy version is 3.0-dev2-d480b7-58
> > [NOTICE] (23315) : path to executable is ./haproxy
> > [DIAG] (23315) : config : parsing [diag.cfg:15] : pattern '//' looks
> > like a failed attempt at commenting an end of line
> > [DIAG] (23315) : config : parsing [diag.cfg:16] : pattern 'and' looks
> > like a failed attempt at using an operator inside a pattern list
> > [...]
>
> That already sounds very helpful.
Thanks for the feedback! I'm going to merge this then.
> I haven't checked locally but adding an
> option similar to zero-warning for diagnostics warnings could be nice on top
> of it; depends on how many FPs happen in practice
It does already exist, with -dW. In fact the diag mode enables emission
of more warnings than the default one, and purposely takes the risk of
emitting false positives which the user has to sort out. You should see
it as a hint like "have you seen this, are you sure this is expected?"
and nothing more. Regular warnings are instead about stuff that ought
to be written differently and that can always be written differently.
But both are subject to -dW.
I checked here, and among 435 test configs that emit zero warnings, only
15 are caught by -dD, essentially due to "weight 0" on a server line, or
due to a "global" section placed after another section (typically after
a "ring" one for traces). One of them was interesting:
bind /tmp/ux shards by-thread
gives:
[crash1.cfg:12]: Disabling per-thread sharding for listener in proxy
'front' because SO_REUSEPORT is disabled
I totally forgot about that and it's a typical use case of diag checks.
Cheers,
Willy
