Re: [PATCH 2/6] libxl: introduce libxl__device_generic_add_t

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

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 ?

> +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 ?

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.