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 10:57:16 +0200

> I went with changing the type to size_t to match what imsg_get_len(3)
> gives us. Since that function can't fail, it feels odd to do an error
> check that looks like checking for a failed function call.

Sure, that's a cleaner approach.

> 
> >
> > I'd need to make a second pass with a fresh head
> 
> here is an updated diff for your fresh head ;)

Looks all good now. Some tests with non-trivial configs would be nice,
but no objection if you want to drop it in and fix things in tree. I
don't think it's a big risk.

ok tb

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