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.
>
>> /* Reserve slots env[0] and env[1]. */
>> p = strdup ("LISTEN_PID=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx");
>> @@ -90,7 +90,6 @@ prepare_socket_activation_environment (string_vector *env)
>> return 0;
>>
>> err:
>> - set_error (errno, "malloc");
>> string_vector_empty (env);
>> return -1;
>> }
>> @@ -99,7 +98,7 @@ STATE_MACHINE {
>> CONNECT_SA.START:
>> int s;
>> struct sockaddr_un addr;
>> - string_vector env = empty_vector;
>> + string_vector env;
>> pid_t pid;
>>
>> assert (!h->sock);
>> @@ -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),
- 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).
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.
(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.
>
>> close (s);
>> return 0;
>> }
>>
>> _______________________________________________
>> Libguestfs mailing list
>> Libguestfs@redhat.com
>> https://listman.redhat.com/mailman/listinfo/libguestfs
>
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
