Re: [PATCH v6 06/18] libxl: write qemu arguments into separate xenstore keys

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Mon, May 18, 2020 at 10:20 AM Ian Jackson <ian.jackson@xxxxxxxxxx> wrote:
>
> Jason Andryuk writes ("[PATCH v6 06/18] libxl: write qemu arguments into 
> separate xenstore keys"):
> > From: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> ...
> > +static int libxl__write_stub_linux_dm_argv(libxl__gc *gc,
> > +                                           int dm_domid, int guest_domid,
> > +                                           char **args)
> > +{
>
> Thanks for the changes.
>
> > +    xs_transaction_t t = XBT_NULL;
> ...
> > +    rc = libxl__xs_read_mandatory(gc, XBT_NULL,
> > +                                  GCSPRINTF("/local/domain/%d/vm", 
> > guest_domid),
> > +                                  &vm_path);
> > +    if (rc)
> > +        return rc;
>
> I think this should be "goto out".  That conforms to standard libxl
> error handling discipline and avoids future leak bugs etc.
> libxl__xs_transaction_abort is a no-op with t==NULL.
>
> Also, it is not clear to me why you chose to put this outside the
> transaction loop.  Can you either put it inside the transaction loop,
> or produce an explanation as to why this approach is race-free...

I just matched the old code's transaction only around the write.  "vm"
shouldn't change during runtime, but I can make the changes as you
suggest.

-Jason