On Tue, Feb 21, 2023 at 05:11:53PM +0100, Laszlo Ersek wrote: > On 2/21/23 14:24, Richard W.M. Jones wrote: > > 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 <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? > >> > >> 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? > > Apologies, I don't understand. > > I think you may be asking one of two questions: > > (1) is it useful to move the zeroing of "env" into > prepare_socket_activation_environment()?
So I agree with this one, but ... > (2) if we decide that (1) is useful, then is the "cast-like" > (string_vector) construct necessary? ... this was my question and ... > The answer to (2) is absolutely "yes"; if we don't put (string_vector) > in front of "empty_vector", that is, if we try > > *env = empty_vector; > > then that's not a structure assignment, it is a syntax error. Right, good point, so ACK to this change, thanks for explaining it to me slowly! > The answer to (1) is not "absolute". My *opinion* (as stated in the > commit message) is that yes, "env" should be zeroed in the callee, not > the caller -- because "env" is a purely output parameter for the callee, > not an input-output parameter. That is, pre-patch, the responsibilities > are incorrectly distributed: the caller zeroes, the callee populates, > the caller consumes. This would only make sense if the callee's > population step actually depended on pre-existent information in "env", > but that's not the case. Therefore the right distribution of > responsibilities (in my opinion!) is that the callee should both zero > and populate, and the caller should consume. > > > > >>>> @@ -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. > > I agree. This pattern (invariant) is satisfied both pre-patch and > post-patch. My point concerns the *depth* on the one particular error > path here at which the error should be set. > > > > >> - 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"); > > > > This is the weaker part of my argument. The stronger part (as I see it) > is that set_error(), while it should *indeed* remain the sole unique > set_error() call on the affected error *path*, belongs in the caller, > not the callee -- that is, to a different depth of the same path. That's > all. > > If you disagree with that, then I'll have to drop this patch. Can we keep the first bit (moving the zeroing of *env), and drop this change that moves set_error out of the function? Rich. > Thanks, > Laszlo > > >> 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! > >> Laszlo > > > > Rich. > > -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs