Re: [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(a)redhat.com&gt; >> --- >> 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.

>> @@ -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