[Libguestfs] [libnbd PATCH v3 12/29] socket activation: clean up responsibilities of prep.sock.act.env.()

[Richard W.M. Jones]

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