Re: [Xen-devel] [PATCH v3 2/7] libxl: add a transaction parameter to libxl__device_generic_add

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Wed, 18 Apr 2012, Ian Campbell wrote:
> > diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> > index c7e057d..05909c5 100644
> > --- a/tools/libxl/libxl_device.c
> > +++ b/tools/libxl/libxl_device.c
> > @@ -58,14 +58,14 @@ int libxl__parse_backend_path(libxl__gc *gc,
> >      return libxl__device_kind_from_string(strkind, &dev->backend_kind);
> >  }
> >  
> > -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;
> >  
> >      frontend_path = libxl__device_frontend_path(gc, device);
> >      backend_path = libxl__device_backend_path(gc, device);
> > @@ -81,7 +81,8 @@ int libxl__device_generic_add(libxl__gc *gc, 
> > libxl__device *device,
> >      backend_perms[1].perms = XS_PERM_READ;
> >  
> >  retry_transaction:
> > -    t = xs_transaction_start(ctx->xsh);
> > +    if (create_transaction)
> > +        t = xs_transaction_start(ctx->xsh);
> >      /* FIXME: read frontend_path and check state before removing stuff */
> >  
> >      if (fents) {
> > @@ -101,13 +102,12 @@ retry_transaction:
> >      }
> >  
> >      if (!xs_transaction_end(ctx->xsh, t, 0)) {
> 
> Do we really want to end the transaction for caller provided t? (i.e.
> when create_transaction == False)
> 
> It would seem more expected to me to return to the caller and expect
> them to complete the transaction and perform error handling etc. If the
> caller doesn't have things of its own to do in the transaction then why
> does it have one in hand to pass in?

I think you are right, I'll change it.


> > -        if (errno == EAGAIN)
> > +        if (errno == EAGAIN && create_transaction)
> >              goto retry_transaction;
> >          else
> >              LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "xs transaction 
> > failed");
> >      }
> > -
> > -    return 0;
> > +    return ERROR_FAIL;
> 
> Where is the success exit path in this function now?

Good point, I'll fix that too.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel