Hi Willy!
On Mon, Apr 13, 2026 at 03:50:24PM +0200, Willy Tarreau wrote:
> On Mon, Apr 13, 2026 at 02:19:38PM +0200, William Lallemand wrote:
> > On Sat, Apr 11, 2026 at 04:03:43PM +0000, Egor Shestakov wrote:
> > > Subject: [PATCH] MINOR: errors: remove excessive errmsg checks
> > > I noticed some strange checks for presence of errmsg. Called functions
> > > generate non-empty error message in case of failure, so a non-NULL address
> > > of the error message is enough.
> > >
> > > No backport needed.
> >
> > I'm not seeing this as excessive, it just avoids emitting a message with an
> > empty string, even if it does not seem useful this is just a guarantee if
> > the
> > message emission is not done correctly. I don't think it's useful to remove
> > them. Ideally we should probably wrap the error generation in some function
> > which would that itself everywhere... But that's another story.
>
> In fact that was not the goal of these tests, it the result of a messed
> up copy-paste from the same test appearing everywhere we use *errmsg like
> this:
>
> @@ -164,6 +188,7 @@ int sockpair_bind_receiver(struct receiver *rx, char
> **errmsg)
> if (errmsg && *errmsg)
> memprintf(errmsg, "%s for [fd %d]", *errmsg, rx->fd);
>
> And by mimetism these check have slipped everywhere a char * is used
> instead of a char **. Take sockpair_bind_receiver() for example:
>
> int sockpair_bind_receiver(struct receiver *rx, char **errmsg)
> {
>
> if (rx->fd == -1) {
> err |= ERR_FATAL | ERR_ALERT;
> memprintf(errmsg, "sockpair may be only used with inherited
> FDs");
> goto bind_return;
> }
> (...)
>
> bind_return:
> if (errmsg && *errmsg)
> memprintf(errmsg, "%s for [fd %d]", *errmsg, rx->fd);
>
> See ? It's impossible here to get errmsg && !*errmsg because of the
> way we arrive there (memprintf() already used to fill the message).
>
> There remain other places like this with variants such as testing
> for (errmsg && *errmsg) before calling indent_msg() after having
> already filled errmsg() from memprintf().
>
> Overall I think these are not important, and that we'll continue to
> regularly mess up with them because half of the "errmsg" are pointers
> to strings while the other half are strings, yet the condition remains
> the same...
I thought that it was just a copy-paste from such parts. When an errmsg is
pointer to a string, these checks make sense.
> > > diff --git a/src/acme.c b/src/acme.c
> > > index eb7137b4c..437528c08 100644
> > > --- a/src/acme.c
> > > +++ b/src/acme.c
> > > @@ -800,8 +800,8 @@ static int cfg_postsection_acme()
> > > /* tries to open the account key */
> > > if (stat(path, &st) == 0) {
> > > if (ssl_sock_load_key_into_ckch(path, NULL, store->data,
> > > &errmsg)) {
> > > - memprintf(&errmsg, "%s'%s' is present but cannot be
> > > read or parsed.\n", errmsg && *errmsg ? errmsg : NULL, path);
> > > - if (errmsg && *errmsg)
> > > + memprintf(&errmsg, "%s'%s' is present but cannot be
> > > read or parsed.\n", errmsg, path);
> > > + if (errmsg)
> > > indent_msg(&errmsg, 8);
> > > err_code |= ERR_ALERT | ERR_FATAL | ERR_ABORT;
> > > ha_alert("acme: %s\n", errmsg);
> >
> > In my opinion this first part should be in a separate BUG/MINOR patch which
> > should be backported.
>
> Sorry, too late, I just merged it :-(
Race condition:-)
> I can revert it and re-apply it as a separate fix if needed, though it's
> not really important as most of the time it will just print "(null)" for
> empty messages instead of nothing, though I'm not aware of any command
> not emitting empty error messages.
We get the "(null)" in the message in any case (with or without the patch).
There is not bug. My patch doesn't fix something, it just eliminate an
uneeded. This is reason why I didn't split it.
Thanks!
--
Egor Shestakov
egor ascii(0x40) ved1 ascii(0x2E) me
