On 2/21/23 17:22, Richard W.M. Jones wrote:
> 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?
It will create a whole lot of rebase conflicts, but yes, I can try.
Laszlo
>
> 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.
>>>
>
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
