Re: [Xen-devel] [PATCH 2/6] libxl: introduce libxl__device_generic_add_t

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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.
> 

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