[Libguestfs] [libnbd PATCH v3 12/29] socket activation: clean up responsibilities of prep.sock.act.env.()

[Eric Blake]

On Wed, Feb 15, 2023 at 04:39:35PM +0000, Richard W.M. Jones wrote:
> On Wed, Feb 15, 2023 at 03:11:41PM +0100, Laszlo Ersek wrote:
> > prepare_socket_activation_environment() is a construction function that is
> > supposed to fill in a string_vector object from the ground up. Right now
> > it has its responsibilities mixed up in two ways:
> > 
> > - it expects the caller to pass in a previously re-set string_vector,
> > 
> > - if it fails, it calls set_error() internally (with a blanket reference
> >   to "malloc").
> > 
> > Fix both warts:
> > 
> > - pass in an *uninitialized* (only allocated) string vector from the
> >   caller, and initialize it in prepare_socket_activation_environment(),
> > 
> > - move the set_error() call out to the caller.
> > 
> > Signed-off-by: Laszlo Ersek <lersek at redhat.com>
> > ---
> >  generator/states-connect-socket-activation.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/generator/states-connect-socket-activation.c b/generator/states-connect-socket-activation.c
> > index c46a0bf5c0a3..b5e146539cc8 100644
> > --- a/generator/states-connect-socket-activation.c
> > +++ b/generator/states-connect-socket-activation.c
> > @@ -51,7 +51,7 @@ prepare_socket_activation_environment (string_vector *env)
> >    char *p;
> >    size_t i;
> >  
> > -  assert (env->len == 0);
> > +  *env = (string_vector)empty_vector;
> 
> Do you actually need to cast this?

Elsewhere in the code, we overwhelmingly do not use the cast.  C++
might require it, but we're using C.

> 
> >    /* Reserve slots env[0] and env[1]. */
> >    p = strdup ("LISTEN_PID=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx");
> > @@ -90,7 +90,6 @@ prepare_socket_activation_environment (string_vector *env)
> >    return 0;
> >  
> >   err:
> > -  set_error (errno, "malloc");

On a rough level, this was correct but unspecific (we only expect
failure due to memory allocations, but didn't do the allocation
locally and don't know which function allocated).

> >    string_vector_empty (env);
> >    return -1;
> >  }
> > @@ -99,7 +98,7 @@ STATE_MACHINE {
> >   CONNECT_SA.START:
> >    int s;
> >    struct sockaddr_un addr;
> > -  string_vector env = empty_vector;
> > +  string_vector env;
> >    pid_t pid;
> >  
> >    assert (!h->sock);
> > @@ -156,6 +155,7 @@  CONNECT_SA.START:
> >  
> >    if (prepare_socket_activation_environment (&env) == -1) {
> >      SET_NEXT_STATE (%.DEAD);
> > +    set_error (errno, "prepare_socket_activation_environment");
> 
> Why move this out of the function?

Moving it here lets us give a more specific message about a function
at a different layer in the stack.  Most memory failures are already
going to be a pain where we don't know if the caller will ever get
back a desired error message, so being inspecific doesn't necessarily
hurt.  But I also don't see any technical reasons to avoid this patch.

Reviewed-by: Eric Blake <eblake at redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org