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

[Laszlo Ersek]

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

(2) if we decide that (1) is useful, then is the "cast-like"
(string_vector) construct necessary?

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.

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.

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