7:17 a.m.
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.
> /* 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(a)redhat.com
> https://listman.redhat.com/mailman/listinfo/libguestfs