Re: [Xen-devel] [PATCH v2 05/15] libxl: change libxl__ao_device_remove to libxl__ao_device

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2012-05-22 at 16:10 +0100, Ian Jackson wrote:
> >  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.
> 
> 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)

[...]

> 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   }

Weirdly (given they were ordered by your preference) I have a preference
for either the first or last form (which I have left quoted above).

I probably slightly prefer the second option, but only slightly
(probably due to a general disquiet about macros which define
functions).

Ian.


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