From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefano Stabellini Subject: Re: [PATCH 2/6] libxl: introduce libxl__device_generic_add_t Date: Fri, 30 Mar 2012 11:46:39 +0100 Message-ID: References: <1332856772-30292-2-git-send-email-stefano.stabellini@eu.citrix.com> <20337.61376.319377.655549@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20337.61376.319377.655549@mariner.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 Jackson Cc: "xen-devel@lists.xensource.com" , Ian Campbell , Stefano Stabellini List-Id: xen-devel@lists.xenproject.org On Tue, 27 Mar 2012, Ian Jackson wrote: > Stefano Stabellini writes ("[PATCH 2/6] libxl: introduce libxl__device_generic_add_t"): > > Introduce libxl__device_generic_add_t that takes an xs_transaction_t as > > parameter. > > Use libxl__device_generic_add_t to implement libxl__device_generic_add. > > Wait, what ? When I suggested these wrappers I thought we were > talking about externally-facing functions which aren't allowed to have > libxenstore types in their signatures. > > For internal functions why not just add the parameter always and allow > the callers to pass 0 ? OK, I'll do that. > > +retry_transaction: > > + t = xs_transaction_start(ctx->xsh); > > + > > + rc = libxl__device_generic_add_t(gc, t, device, bents, fents); > > + > > + if (!xs_transaction_end(ctx->xsh, t, 0)) { > > + if (errno == EAGAIN) > > + goto retry_transaction; > > + else > > + LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "xs transaction failed"); > > + } > > + return rc; > > Can we avoid introducing more of these transaction loops using goto ? I am afraid I actually prefer the simple goto scheme than the convoluted code below. > How about we invent some helper functions: > > int libxl__xs_transaction_init(libxl__gc *gc, > xs_transaction_t **my_out, > xs_transaction_t *from_caller) { > if (from_caller) { *my_out = from_caller; return 0; } > > *my_out = get new transaction or log error, set rc; > return rc; > } > > int libxl__xs_transaction_done(libxl__gc *gc, > xs_transaction_t **my_io) { > xs_transaction_t *from_caller) { > if (from_caller) { *my_out = 0; return 0; } > > r = xs_transaction_end blah; > if (!r) { *my_io = 0; return 0; } > if (errno != EAGAIN) { log; report error; } > return libxl__xs_transaction_get(gc, my_io, from_caller); > } > > void libxl__xs_transaction_cleanup(libxl__gc *gc, xs_transaction_t **my) { > xs_transaction_end(,,1) plus some error logging; > } > > #define LOOP_WITH_XS_TRANSACTION(my_t, from_caller) \ > for (({ \ > (rc) = libxl__xs_transaction_get((gc),(&my_t),(from_caller)) \ > if ((rc)) goto error_out; \ > }); \ > (my_t); \ > ({ \ > rc = libxl__xs_transaction_done(gc,&(my_t),(from_caller)); \ > if (rc) goto out; \ > }) > > Then this: > > > +retry_transaction: > > + t = xs_transaction_start(ctx->xsh); > > + > > + STUFF > > + > > + if (!xs_transaction_end(ctx->xsh, t, 0)) { > > + if (errno == EAGAIN) > > + goto retry_transaction; > > + else > > + LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "xs transaction failed"); > > + } > > + return rc; > > Turns into: > > LOOP_WITH_XS_TRANSACTION(t, caller_t) { > STUFF > } > ... > > error_out: > libxl__xs_transaction_cleanup(gc,t); > > And as a bonus you can declare the function to take an > xs_transaction_t so that it can be called either from within such a > loop, or standalone. > > If we were feeling really advanced we could do away with the cleanup > if we put the transaction in the gc. > > > Ian. >