Re: [Xen-devel] [PATCH v4 01/10] libxl: change libxl__ao_device_remove to libxl__ao_device

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Roger Pau Monne writes ("[PATCH v4 01/10] libxl: change libxl__ao_device_remove 
to libxl__ao_device"):
> Introduce a new structure to track state of device backends, that will
> be used in following patches on this series.
> 
> This structure if used for both device creation and device
> destruction and removes libxl__ao_device_remove.
...
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index e3d05c2..7fdecf1 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
...
> +/* Macro for defining device remove/destroy functions in a compact way */
> +#define DEFINE_DEVICE_REMOVE(type, removedestroy, f)                    \

Looks reasonable to me.

> +/* Define all remove/destroy functions and undef the macro */
> +
> +/* disk */
> +DEFINE_DEVICE_REMOVE(disk, remove, 0)
> +DEFINE_DEVICE_REMOVE(disk, destroy, 1)

I'm not sure what purpose the comment serves but I don't really object
to it :-).

> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 52f5435..670a17b 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
...
> @@ -830,13 +830,6 @@ _hidden const char *libxl__device_nic_devname(libxl__gc *
> +/* This functions sets the necessary libxl__ao_device struct values to use
> + * safely inside functions. It marks the operation as "active"
> + * since we need to be sure that all device status structs are set
> + * to active before start queueing events, or we might call
> + * ao_complete before all devices had finished */
> +_hidden void libxl__prepare_ao_device(libxl__ao_device *aodev, libxl__ao *ao,
> +                                      libxl__ao_device **base);

You need to clearly explain what the constraints are on the order of
calling _prepare and _initiate_..._remove.

Questions whose answers should be clear from your text include:
 - is it legal to call _initiate_ without having previously called
   _prepare ?
 - is it legal to call _prepare and then simply throw away the
   aodev ?
 - is it legal to call _prepare multiple times ?  (If the answer
   to the previous question is `yes' then it must be, I think.)

> @@ -436,34 +441,35 @@ out:
> -int libxl__initiate_device_remove(libxl__egc *egc, libxl__ao *ao,
> -                                  libxl__device *dev)
> +void libxl__initiate_device_remove(libxl__egc *egc,
> +                                   libxl__ao_device *aodev)
>  {
...
> +retry_transaction:
> +    t = xs_transaction_start(ctx->xsh);
> +    if (aodev->force)
> +        libxl__xs_path_cleanup(gc, t,
> +                               libxl__device_frontend_path(gc, aodev->dev));
> +    state = libxl__xs_read(gc, t, state_path);

Didn't I previously comment adversely on having this check here ?

Ah yes, I commented on the corresponding code in add (v2 08/15):

  [...], I think all of this is done for you by
  libxl__ev_devstate_wait.  You don't need to pre-check the state path
  at all.

And:

  Do you really need to do the xenstore state read here ?  Surely
  libxl__ev_devstate_wait will do it for you.

> +    /* Some devices (vkbd) fail to disconnect properly,
> +     * but we shouldn't alarm the user if it's during
> +     * domain destruction.
> +     */
> +    if (rc && aodev->action == DEVICE_CONNECT) {
> +        LOG(ERROR, "unable to connect device with path %s",
> +                   libxl__device_backend_path(gc, aodev->dev));
> +        goto out;
> +    } else if(rc) {

Missing space after `if'.

> +        LOG(DEBUG, "unable to disconnect device with path %s",
> +                   libxl__device_backend_path(gc, aodev->dev));
> +        goto out;
> +    }

Last time I wrote:

  Perhaps we should have a separate flag which controls error reporting
  so that a user-requested graceful device disconnect gets full error
  logging ?

Did you miss the fact that email suggesting the macro for generating
the device remove functions also had these other comments in ?

Thanks,
Ian.

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