Richard W.M. Jones
2023-Feb-15 16:39 UTC
[Libguestfs] [libnbd PATCH v3 12/29] socket activation: clean up responsibilities of prep.sock.act.env.()
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?> /* 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"); > 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? Rich.> close (s); > return 0; > } > > _______________________________________________ > Libguestfs mailing list > Libguestfs at redhat.com > https://listman.redhat.com/mailman/listinfo/libguestfs-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
Eric Blake
2023-Feb-17 18:36 UTC
[Libguestfs] [libnbd PATCH v3 12/29] socket activation: clean up responsibilities of prep.sock.act.env.()
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
Laszlo Ersek
2023-Feb-21 13:17 UTC
[Libguestfs] [libnbd PATCH v3 12/29] socket activation: clean up responsibilities of prep.sock.act.env.()
On 2/15/23 17:39, 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?This is not a cast, but a C99 compound literal. And yes, it is necessary, as empty_vector is just: #define empty_vector { .ptr = NULL, .len = 0, .cap = 0 } So this is *not* initialization, but assignment. We have a string_vector object (a structure) on the LHS, so we ned a structure on the RHS as well. The compound literal provides that (unnamed, automatic storage duration) structure. It looks like a cast (quite intentionally, I'd hazard), but it's not a cast.> >> /* 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"); >> 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?Two reasons: - in the caller (CONNECT_SA.START handler), every other failure branch calls set_error explicitly (and subsequent patches in the series will uphold the same pattern), - as the commit message says, the blanket "malloc" reference in prepare_socket_activation_environment() is not accurate enough, and certainly will not be accurate any longer with later patches (e.g. patch #26, which returns -1/EOVERFLOW upon ADD_OVERFLOW() failing). Note that in patch #19, a very similar cleanup is performed for CONNECT_COMMAND.START; there, we supply a missing set_error() for fcntl(), plus a *comment* that nbd_internal_socket_create() sets the error internally. (I disagree with nbd_internal_socket_create() setting the error internally, but that function is too widely called to move set_error() out of it, to all its callers, and again I needed to contain the scope creep. So, for at least restoring the "visual" uniformity of set_error() calls in CONNECT_COMMAND.START, I added the comment.) Thanks! Laszlo> > Rich. > >> close (s); >> return 0; >> } >> >> _______________________________________________ >> Libguestfs mailing list >> Libguestfs at redhat.com >> https://listman.redhat.com/mailman/listinfo/libguestfs >