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

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

Ian Jackson wrote:
> 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.)
I've added a more detailed comment about _prepare.

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

> > +    /* 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'.
Done.

> > +        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 ?
I've replied to this email, the response is here:

http://marc.info/?l=xen-devel&m=133770109429587&w=2

I would like to leave this for future work, since the error checking we 
have now is not better that what I'm proposing here.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel