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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2012-03-27 at 14:59 +0100, Stefano Stabellini wrote:
> 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.

I think it would be better to just change the existing API to add a
transaction.

> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> ---
>  tools/libxl/libxl_device.c   |   36 ++++++++++++++++++++++++++----------
>  tools/libxl/libxl_internal.h |    2 ++
>  2 files changed, 28 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index c2880e0..c6f9044 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -61,12 +61,37 @@ int libxl__parse_backend_path(libxl__gc *gc,
>  int libxl__device_generic_add(libxl__gc *gc, libxl__device *device,
>                               char **bents, char **fents)
>  {
> +    int rc = 0;
> +    xs_transaction_t t;
> +    libxl_ctx *ctx = libxl__gc_owner(gc);
> +
> +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;
> +}
> +
> +int libxl__device_generic_add_t(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];
>  
> +    if (t == XBT_NULL)
> +        /* we need a valid transaction: call libxl__device_generic_add
> +         * to create one for us */
> +        return libxl__device_generic_add(gc, device, bents, fents);

This function will in turn start a new transaction and then call us
straight back again, which is all a bit of a roundabout way to do
things. Can't this just be a t = xs_transaction_start?

> +
>      frontend_path = libxl__device_frontend_path(gc, device);
>      backend_path = libxl__device_backend_path(gc, device);
>  
> @@ -80,8 +105,6 @@ int libxl__device_generic_add(libxl__gc *gc, libxl__device 
> *device,
>      backend_perms[1].id = device->domid;
>      backend_perms[1].perms = XS_PERM_READ;
>  
> -retry_transaction:
> -    t = xs_transaction_start(ctx->xsh);
>      /* FIXME: read frontend_path and check state before removing stuff */
>  
>      if (fents) {
> @@ -100,13 +123,6 @@ retry_transaction:
>          libxl__xs_writev(gc, t, backend_path, bents);
>      }
>  
> -    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 0;
>  }
>  
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index e0a1070..f97213f 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -667,6 +667,8 @@ _hidden int libxl__device_console_add(libxl__gc *gc, 
> uint32_t domid,
>  
>  _hidden int libxl__device_generic_add(libxl__gc *gc, libxl__device *device,
>                               char **bents, char **fents);
> +_hidden int libxl__device_generic_add_t(libxl__gc *gc, xs_transaction_t t,
> +        libxl__device *device, char **bents, char **fents);
>  _hidden char *libxl__device_backend_path(libxl__gc *gc, libxl__device 
> *device);
>  _hidden char *libxl__device_frontend_path(libxl__gc *gc, libxl__device 
> *device);
>  _hidden int libxl__parse_backend_path(libxl__gc *gc, const char *path,



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