On Mon, Apr 13, 2026 at 03:50:24PM +0200, Willy Tarreau wrote:
> Subject: Re: [PATCH] MINOR: errors: remove excessive errmsg checks
> 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...
>
> > > 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 :-( 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.
>
I misread this particular part, I thought it introduced the check, so that's
fine.
--
William Lallemand
