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?
> >> @@ -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.
> - 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");
> 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.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
