Re: [PATCH v2 05/15] 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 v2 05/15] 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_internal.h b/tools/libxl/libxl_internal.h
>> index ae5527b..b62110a 100644
>> --- a/tools/libxl/libxl_internal.h
>> +++ b/tools/libxl/libxl_internal.h
>> @@ -1802,6 +1795,44 @@ _hidden void libxl__bootloader_init(libxl__bootloader_state *bl);
> ...
>> +struct libxl__ao_device {
>> +    /* filled in by user */
>> +    libxl__ao *ao;
>> +    /* action being performed */
>> +    libxl__device_action action;
>> +    libxl__device *dev;
>> +    int force;
>> +    libxl__device_callback *callback;
>> +    /* private for implementation */
>> +    /* State in which the device is */
>> +    int active;
>> +    int rc;
>> +    libxl__ev_devstate ds;
>> +    void *base;
>> +};
>
> Re your comment, don't you mean "state of the operation" rather than
> "state of the device" ?
>
> I would prefer some reformatting to make the scope of comments like
> "filled in by user" clearer.  Perhaps a blank line before "private for
> implementation".  Or perhaps moving all the small comments to after
> the variables.
>
> TBH I'm not sure what the comment "action being performed" is for.
> After all the variable is called "action" so it's obvious.

No, probably no need to write any comment, since it's obvious.

>> +/* Arranges that dev will be removed to the guest, and the
>> + * hotplug scripts will be executed (if necessary). When
>> + * this is done (or an error happens), the callback in
>> + * aorm->callback will be called.
>> + */
>> +_hidden void libxl__initiate_device_remove(libxl__egc *egc,
>> +                                           libxl__ao_device *aorm);
>> +
>
> Does this really only do removals ?  If so where is the addition
> function and what is the "action" parameter for ?

The addition code comes in a next patch, but I've already added the 
"action" libxl__ao_device var to reduce the noise, since it's a harmless 
addition.

>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> index 2a09192..b13de4f 100644
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c
>> @@ -1271,6 +1271,26 @@ int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid, int autopass)
>>
>>   /******************************************************************************/
>>
>> +/* generic callback for devices that only need to set ao_complete */
>> +static void libxl__device_cb(libxl__egc *egc, libxl__ao_device *aorm)
>
> Maybe this should have a more descriptive name ?
>     device_addrm_aocomplete
> depending on whether it's for removal only or for addition too.

It's for both addition/removal.

> You are still using the parameter name "aorm" for something which is
> not only a removal operation.  "aodev" or something.
 >
>>   int libxl_device_nic_destroy(libxl_ctx *ctx, uint32_t domid,
>> -                                  libxl_device_nic *nic)
>> +                             libxl_device_nic *nic,
>> +                             const libxl_asyncop_how *ao_how)
>>   {
>> -    GC_INIT(ctx);
>> -    libxl__device device;
>> +    AO_CREATE(ctx, domid, ao_how);
>> +    libxl__device *device;
>> +    libxl__ao_device *aorm;
>>       int rc;
>>
>> -    rc = libxl__device_from_nic(gc, domid, nic,&device);
>> +    GCNEW(device);
>> +    rc = libxl__device_from_nic(gc, domid, nic, device);
>>       if (rc != 0) goto out;
>>
>> -    rc = libxl__device_destroy(gc,&device);
>> +    GCNEW(aorm);
>> +    libxl__init_ao_device(aorm, ao, NULL);
>> +    aorm->action = DEVICE_DISCONNECT;
>> +    aorm->force = 1;
>> +    aorm->dev = device;
>> +    aorm->callback = libxl__device_cb;
>> +    libxl__initiate_device_remove(egc, aorm);
>> +
>
> Is there some way to make these functions less repetitive ?
> We have {nic,disk,vkb,vfb}_{remove,destroy} all of which have
> essentially identical innards.
>
> I have some suggestions, in descending order of my own preference.

Well, I have to say I prefer the first option, since it reduces the 
length of code, but I don't know if it will be too complicated. Is it ok 
if I include this in this patch, or should I put it on a separate patch?

> How about writing the common code once in a macro:
>
>   1  #define DEFINE_DEVICE_REMOVE(type, removedestroy, force)       \
>   1     int libxl_device_##type##_##removedestroy(libxl_ctx *ctx,   \
>   1                 uint32_t domid, libxl_device_##type *nic,       \
>   1                 const libxl_asyncop_how *ao_how)                \
>   1     {                                                           \
>   1         AO_CREATE etc. etc.                                     \
>   1         rc = libxl__device_from_##type( etc. etc. )             \
>   1         etc. etc.                                               \
>   1     }
>
>   8  DEFINE_DEVICE_REMOVE(nic, remove, 0)
>   .  DEFINE_DEVICE_REMOVE(nic, destroy, 1)
>   .  DEFINE_DEVICE_REMOVE(disk, remove, 0)
>   .  DEFINE_DEVICE_REMOVE(disk, destroy, 1)
>   .  DEFINE_DEVICE_REMOVE(vkb, remove, 0)
>   .  DEFINE_DEVICE_REMOVE(vkb, destroy, 1)
>   .  DEFINE_DEVICE_REMOVE(vfb, remove, 0)
>   .  DEFINE_DEVICE_REMOVE(vfb, destroy, 1)
>
> (Numbers on the left show how many instances of formulaic code there
> are.)
>
> Or writing the common code once in a function which takes an
> appropriate function pointer type for the conversion:
>
>   1  int device_remove(libxl_ao *ao, uint32_t domid, int force,
>   1                    void *libxl_foo_device,
>   1                    int (*libxl_device_from_foo_device)
>   1                         (libxl__gc *gc, uint32_t domid,
>   1                          void *libxl_foo_device, libxl__device **device))
>   1  {
>   1      AO_CREATE etc. etc.
>
>   4  static int device_from_nic_void(libxl__gc *gc, uint32_t domid,
>   4                          void *libxl_foo_device, libxl__device **device)
>   4      { return libxl__device_from_nic(gc, domid, libxl_foo_device, device); }
>
>   8  int libxl_device_nic_remove(libxl_ctx *ctx, uint32_t domid,
>   8                               libxl_device_nic *nic,
>   8                               const libxl_asyncop_how *ao_how)
>   8      { return device_remove(ctx, domid, 0, nic, device_from_nic_void); }
>
>   .  int libxl_device_nic_destroy(libxl_ctx *ctx, uint32_t domid,
>   .                               libxl_device_nic *nic,
>   .                               const libxl_asyncop_how *ao_how)
>   .      { return device_remove(ctx, domid, 1, nic, device_from_nic_void); }
>
> Or combining the above:
>
>   1  int device_remove(libxl_ao *ao, uint32_t domid, int force,
>   1                    void *libxl_foo_device,
>   1                    int (*libxl_device_from_foo_device)
>   1                         (libxl__gc *gc, uint32_t domid,
>   1                          void *libxl_foo_device, libxl__device **device))
>   1  {
>   1      AO_CREATE etc. etc.
>
>   1  #define DEFINE_DEVICE_REMOVE(type)                                      \
>   4    static int device_from_##type##_void(libxl__gc *gc, uint32_t domid,   \
>   4                          void *libxl_foo_device, libxl__device **device) \
>   4        { return libxl__device_from_##type(gc, domid,                     \
>   4                                libxl_foo_device, device); }              \
>                                                                              \
>   2  int libxl_device_##type##_remove(libxl_ctx *ctx, uint32_t domid,        \
>   2                               libxl_device_##type *thing,                \
>   2                               const libxl_asyncop_how *ao_how)           \
>   2      { return device_remove(ctx, domid, 0, thing,                        \
>   2                             device_from_##type##_void); }                \
>                                                                              \
>   .  int libxl_device_##type##_destroy(libxl_ctx *ctx, uint32_t domid,       \
>   .                               libxl_device_##type *thing,                \
>   .                               const libxl_asyncop_how *ao_how)           \
>   .      { return device_remove(ctx, domid, 1, thing,                        \
>   .                             device_from_##type##_void); }
>
>   4  DEVICE_DEVICE_REMOVE(nic)
>   .  DEVICE_DEVICE_REMOVE(disk)
>   .  DEVICE_DEVICE_REMOVE(vfb)
>   .  DEVICE_DEVICE_REMOVE(vkb)
>
> Or at least reducing the number of copies from 8 to 4 like this:
>
>   4  int nic_remove(libxl_ctx *ctx, uint32_t domid,
>   4                 libxl__device_nic *nic, int force,
>   4                 const libxl_asyncop_how *ao_how);
>
>   8  int libxl_device_nic_remove(libxl_ctx *ctx, uint32_t domid,
>   8                               libxl_device_nic *nic,
>   8                               const libxl_asyncop_how *ao_how)
>   8    { return nic_remove(ctx, domid, nic, 0, ao_how); }
>
>   .  int libxl_device_nic_destroy(libxl_ctx *ctx, uint32_t domid,
>   .                               libxl_device_nic *nic,
>   .                               const libxl_asyncop_how *ao_how)
>   .      { return nic_remove(ctx, domid, nic, 1, ao_how); }
>
> Or at least reducing the size of the octuplicated code like this:
>
>   1  int device_remove(libxl_ao *ao, uint32_t domid,
>   1                    libxl__device *device, int force)
>   1  {
>   1    ...
>   1    GCNEW(aodev);
>   1    libxl__init_ao_device(aodev, ao, NULL);
>   1    aodev->action = DEVICE_DISCONNECT;
>   1    aodev->force = 1;
>   1    aodev->dev = device;
>   1    aodev->callback = libxl__device_cb;
>   1    libxl__initiate_device_addremove(egc, aodev);
>   1    etc. etc.
>
>   8  int libxl_device_nic_remove(libxl_ctx *ctx, uint32_t domid,
>   8                              libxl_device_nic *nic,
>   8                              const libxl_asyncop_how *ao_how)
>   8  {
>   8      AO_CREATE(ctx, domid, ao_how);
>   8      int rc;
>   8      libxl__device *device;
>   8
>   8      rc = libxl__device_from_nic(gc, domid, nic,&device);
>   8      if (rc) return AO_ABORT(rc);
>   8
>   8      device_remove(ao, domid, device, 0);
>   8      return AO_INPROGRESS;
>   8   }
>
> ?
>
>> +/* Device AO operations */
>>
>> -typedef struct {
>> -    libxl__ao *ao;
>> -    libxl__ev_devstate ds;
>> -} libxl__ao_device_remove;
>> +void libxl__init_ao_device(libxl__ao_device *aorm, libxl__ao *ao,
>> +                           libxl__ao_device **base)
>> +{
>> +    aorm->ao = ao;
>> +    aorm->active = 1;
>> +    aorm->rc = 0;
>> +    aorm->base = base;
>> +}
>
> I think a function "init" should set "active" to 0, surely ?
> Since the operation has not in fact been started.  So perhaps this
> should just be a memset.
>
>> +retry_transaction:
>> +    t = xs_transaction_start(ctx->xsh);
>> +    if (aorm->force)
>> +        libxl__xs_path_cleanup(gc, t,
>> +                               libxl__device_frontend_path(gc, aorm->dev));
>> +    state = libxl__xs_read(gc, t, state_path);
>>       if (!state)
>>           goto out_ok;
>
> If you're reworking this, you should check for errors other than
> ENOENT here.
>
> But actually, 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.
>
> Also you seem to only do the _path_cleanup thing (removing newly empty
> parent directories) in the force case, if I'm not mistaken.  I think
> it would be better to make the force case simply skip the device wait
> and then reuse the rest of the code path if possible.

Right now it is true that the path_cleanup thing is only used when using 
device_{...}_remove/destroy functions, but later patches change that. It 
is not modified here because I wanted to keep the changes that touch 
libxl__devices_destroy together.

Not really, at least for vif we actually need to wait for the device to 
switch to state 6 or we will get error messages on xenstore that come 
from the fact that the kernel is also monitoring xenstore and it looses 
track of the vif interface before it's properly shut down.

In the past we never saw this errors because xen-hotplug-cleanup script 
deletes them from xenstore (nice).

>> +    if (rc == ERROR_TIMEDOUT&&  aorm->action == DEVICE_DISCONNECT&&
>> +        !aorm->force) {
>> +        aorm->force = 1;
>> +        libxl__initiate_device_remove(egc, aorm);
>> +        return;
>> +    }
>
> I like this approach.  Economical.
>
>> +    /* Some devices (vkbd) fail to disconnect properly,
>> +     * but we shouldn't alarm the user if it's during
>> +     * domain destruction.
>> +     */
>
> Perhaps we should have a separate flag which controls error reporting
> so that a user-requested graceful device disconnect gets full error
> logging ?

Can we left that for a next patch?

I would like this series to not keep growing indefinitely.

> Thanks,

Thanks for the review!