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 >
Richard W.M. Jones
2023-Feb-21 13:24 UTC
[Libguestfs] [libnbd PATCH v3 12/29] socket activation: clean up responsibilities of prep.sock.act.env.()
On Tue, Feb 21, 2023 at 02:17:15PM +0100, Laszlo Ersek wrote:> 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.OK it's not a cast, but struct assignment is well defined so is the change necessary?> >> @@ -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),The pattern is actually that we call set_error once on each error path [which is surprisingly hard to get right -- we've even tried to write verifier tools for this in the past]. If a function f() calls function g(), where the g() will call set_error, then there's no need for function f() to call set_error on that path. That applies even if there are other disjoint paths where function f() calls set_error, eg. because f() calls malloc directly.> - 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).I'm unconvinced, couldn't you change the original message to be something like this? set_error (errno, "prepare_socket_activation_environment: malloc");> 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.Adding missing calls to set_error is good, no problem with that.> (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! > LaszloRich. -- 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