11:56 a.m.
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 <lersek(a)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.
>>