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.
Willy
