From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefano Stabellini Subject: Re: [PATCH v3 2/7] libxl: add a transaction parameter to libxl__device_generic_add Date: Fri, 20 Apr 2012 16:13:52 +0100 Message-ID: References: <1334601190-14187-2-git-send-email-stefano.stabellini@eu.citrix.com> <1334738510.23948.145.camel@zakaz.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1334738510.23948.145.camel@zakaz.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: "xen-devel@lists.xensource.com" , Ian Jackson , Stefano Stabellini List-Id: xen-devel@lists.xenproject.org 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.