On 2023/11/16 22:03:16 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> On Wed, Nov 15, 2023 at 07:46:57PM +0100, Omar Polo wrote:
> > updated diff, this includes the fix to the daemon() call too.
> 
> Thanks for working on this! I read through and didn't see anything
> horrible. Ok by me. We'll deal with any fallout that comes up later.
> I will hold off a new release for a few days to give developers some
> time to test it.
> 
> > +int
> > +main_compose_sockets(struct gotwebd *env, uint32_t type, int fd,
> > +    const void *data, uint16_t len)
> > +{
> > +	size_t	 i;
> > +	int	 ret, d;
> > +
> > +	for (i = 0; i < env->nserver; ++i) {
> > +		d = -1;
> > +		if (fd != -1 && (d = dup(fd)) == -1)
> > +			return (-1);
> 
> This function does not close fd on error, but closes fd on success.
> It's not a big deal since all callers end up in fatal() if this
> function fails, and this code is only used while loading the
> configuration. But it looks odd when some return paths care
> about closing the file and some don't.

I've committed my original diff and fixed this in a follow-up.

> [...]
> > +	if (p[0] != 3) {
> > +		if (dup2(p[0], 3) == -1)
> 
> This magic number 3 was derived from PROC_GOTWEBD_SOCK_FILENO, right?
> Consider adding a GOTWEBD_SOCK_FILENO constant for it.

also done in a follow-up.

> [[[]]]
> > +#define IMSG_SIZE_CHECK(imsg, p) do {					\
> > +	if (IMSG_DATA_SIZE(imsg) < sizeof(*p))				\
> 
> Could this check even be != sizeof(*p)?
> As far as I can see all messages used by gotwebd are of a fixed size.

IIRC this define is used by other daemons as well and I'd like not to
change its behaviour here.

Now, actually I'd prefer to remove completely its usage.  I always get
confused about whether it takes a pointer or the variable directly, and
I've fixed at least a few wrong usage (one was fixed in the diff
itself.)