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

[Roger Pau Monne <roger.pau@citrix.com>]

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.