Re: [PATCH v2 07/18] libxl: separate device add/rm complete callbacks

[Ian Campbell <ian.campbell@citrix.com>]

On Wed, 2014-07-30 at 19:23 +0100, Wei Liu wrote:
> This is in preparation for device hotplug / unplug configuration
> synchronisation. No functional change introduced. This change is
> necessary because rm is type specific, using a single callback cannot

Not "is" but "is going to become" I think?

> meet our need.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxl/libxl.c |   54 +++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 42 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 9ac2c3d..01dffa0 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1798,22 +1798,26 @@ out:
>  }
>  
>  /******************************************************************************/
> +#define DEVICE_AO_FAILED_MSG                                            \

I don't see why this couldn't be a static log_ao_failure message, but
since it is used like a function it should be defined/called as
DEVICE_AO_FAILED_MSG() so it looks normal. I think it should just be a
function though.
> @@ -3537,6 +3541,32 @@ out:
>  }
>  
>  /******************************************************************************/
> +#define DEFINE_DEVICE_RM_AOCOMPLETE(type)                               \
> +    static void device_##type##_rm_aocomplete(libxl__egc *egc,          \
> +                                              libxl__ao_device *aodev)  \
> +    {                                                                   \
> +        STATE_AO_GC(aodev->ao);                                         \
> +        if (aodev->rc) {                                                \
> +            DEVICE_AO_FAILED_MSG;                                       \
> +            goto out;                                                   \
> +        }                                                               \
> +                                                                        \
> +    out:                                                                \

Perhaps you are going to add stuff between } and out: in a future patch,
but if not then the goto+label are a bit pointless (I see now that you
inherited that from the original code, so feel free to leave it if you
prefer).

> +        libxl__ao_complete(egc, ao, aodev->rc);                         \
> +    }
> +
> +/*
> + * device_vtpm_rm_aocomplete
> + * device_nic_rm_aocomplete
> + * device_disk_rm_aocomplete
> + * device_vfb_rm_aocomplete
> + * device_vkb_rm_aocomplete
> + */
> +DEFINE_DEVICE_RM_AOCOMPLETE(vtpm);
> +DEFINE_DEVICE_RM_AOCOMPLETE(nic);
> +DEFINE_DEVICE_RM_AOCOMPLETE(disk);
> +DEFINE_DEVICE_RM_AOCOMPLETE(vfb);
> +DEFINE_DEVICE_RM_AOCOMPLETE(vkb);

I was going to suggest merging this with DEFINE_DEVICE_REMOVE but that
is used twice for each device so that won't work. But perhaps consider
adding  these in the same blocks as the usage of DEFINE_DEVICE_REMOVE?