Re: [PATCH v4 3/8] libxl: add a transaction parameter to libxl__device_generic_add

[Ian Jackson <Ian.Jackson@eu.citrix.com>]

Stefano Stabellini writes ("[Xen-devel] [PATCH v4 3/8] libxl: add a transaction parameter to libxl__device_generic_add"):
> Add a xs_transaction_t parameter to libxl__device_generic_add, if it is
> XBT_NULL, allocate a proper one.
...
> -int libxl__device_generic_add(libxl__gc *gc, libxl__device *device,
> -                             char **bents, char **fents)
> +int libxl__device_generic_add(libxl__gc *gc, xs_transaction_t t,
> +        libxl__device *device, char **bents, char **fents)
>  {
>      libxl_ctx *ctx = libxl__gc_owner(gc);
>      char *frontend_path, *backend_path;
> -    xs_transaction_t t;
>      struct xs_permissions frontend_perms[2];
>      struct xs_permissions backend_perms[2];
> +    int create_transaction = t == XBT_NULL;

This works.

Another way to do it is to have a separate variable,
something like this:
   xs_transaction_t our_transaction = t;
and then later
   if (!t) {
       our_transaction = t = xs_transaction_start....
       if (!t) error handling;
   }
and when you commit it set our_transaction to 0 and on error exit
   if (our_transaction)
       xs_transaction_end(..., our_transaction, 1);

I suggest this just because you may prefer to avoid separate boolean
sentinel variables - I know I do.


There is a difficulty in general with this function, which is that it
contains an enormous number of unchecked xenstore operations.  I'm not
saying you need to fix this now, but if at a later point this were to
be fixed, the function would need to get a proper error exit path
which would have to destroy the transaction iff it was created.

I mention this for completeness but you may want to transpose it into
a comment somehow.


Anyway,

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>