From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Jackson <Ian.Jackson@eu.citrix.com>
Subject: Re: [PATCH 2/6] libxl: introduce
	libxl__device_generic_add_t
Date: Tue, 27 Mar 2012 17:50:08 +0100
Message-ID: <20337.61376.319377.655549@mariner.uk.xensource.com>
References: <alpine.DEB.2.00.1203271453390.15151@kaball-desktop>
	<1332856772-30292-2-git-send-email-stefano.stabellini@eu.citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1332856772-30292-2-git-send-email-stefano.stabellini@eu.citrix.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: "xen-devel@lists.xensource.com" <xen-devel@lists.xensource.com>, Ian Campbell <Ian.Campbell@citrix.com>
List-Id: xen-devel@lists.xenproject.org

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.