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 <ler...@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 <ebl...@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
