From: Theo Buehler <tb@theobuehler.org>
Subject: Re: dhcpleased(8): do not peek inside of struct imsg
To: tech <tech@openbsd.org>
Date: Sun, 25 Aug 2024 16:26:04 +0200

On Sun, Aug 25, 2024 at 03:32:54PM +0200, Florian Obser wrote:
> On 2024-08-25 10:57 +02, Theo Buehler <tb@theobuehler.org> wrote:
> >> @@ -491,44 +493,48 @@ engine_dispatch_main(int fd, short event, void *bula)
> >>  			break;
> >>  		case IMSG_RECONF_VC_ID:
> >>  			if (iface_conf == NULL)
> >> -				fatal("IMSG_RECONF_VC_ID without "
> >> +				fatalx("IMSG_RECONF_VC_ID without "
> >>  				    "IMSG_RECONF_IFACE");
> >
> > should these messages include __func__ for consistency with most other
> > fatals in here?
> >
> > I was wondering if it makes sense to make this slightly stricter and add:
> >
> > 			if (iface_conf->vc_id != NULL)
> > 				fatalx("multiple IMSG_RECONF_VC_ID "
> > 				    "for the same interface");
> >
> > otherwise the following malloc() looks like a potential leak.
> >
> > If you do this, of course C_ID and H_NAME should get the same treatment,
> > also in the frontend. All this would better be in a separate changeset.
> >
> 
> All of that makes sense.

cool

ok tb

> (It might be a bit silly to go through i2s, but that results in less
> awkward line breaks, I think.)

I was in fact hesitating to suggest that you do this but it felt too
nitpicky. I think it looks cleaner this way. For consistency I'd do:

			if (nconf == NULL)
				fatalx("%s: %s without IMSG_RECONF_CONF",
				    __func__, i2s(type));

in the two IMSG_RECONF_END cases.