Re: [PATCH V4 22/24] xl: update domain configuration when we hotplug a device

[David Vrabel <david.vrabel@citrix.com>]

On 06/05/14 18:13, Wei Liu wrote:
> On Tue, May 06, 2014 at 05:57:34PM +0100, David Vrabel wrote:
>> On 01/05/14 13:58, Wei Liu wrote:
>>> A convenient macro is written to accomplish following tasks:
>>>
>>> 1. load domain configuration
>>> 2. allocate a new device structure NEW
>>> 3. copy the parsed device strcture PARSED to NEW
>>> 4. call libxl_device_TYPE_add(PARSED)
>>> 5. pull from PARSED any fields that might be touched by libxl to NEW
>>> 6. store domain configuration
>>>
>>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>>> ---
>>>  tools/libxl/xl_cmdimpl.c |   85 +++++++++++++++++++++++++++++++++++++---------
>>>  1 file changed, 69 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>>> index 83f058e..91172c5 100644
>>> --- a/tools/libxl/xl_cmdimpl.c
>>> +++ b/tools/libxl/xl_cmdimpl.c
>>> @@ -381,6 +381,36 @@ static void *xrealloc(void *ptr, size_t sz) {
>>>          &(array)[array_extend_old_count];                               \
>>>      })
>>>  
>>> +#define ARRAY_EXTEND_INIT_NODEVID(array,count,initfn)                   \
>>> +    ({                                                                  \
>>> +        typeof((count)) array_extend_old_count = (count);               \
>>> +        (count)++;                                                      \
>>> +        (array) = xrealloc((array), sizeof(*array) * (count));          \
>>> +        (initfn)(&(array)[array_extend_old_count]);                     \
>>> +        &(array)[array_extend_old_count];                               \
>>> +    })
>>> +
>>> +/* Add a device and update the stored configuration */
>>> +#define ADD_DEVICE(devtype,domid,dev,d_config,ptr,allocate,maybe_fixup) \
>>> +    do {                                                                \
>>> +        libxl_domain_config_init((d_config));                           \
>>> +        load_domain_config((domid), (d_config));                        \
>>> +                                                                        \
>>> +        allocate;                                                       \
>>> +                                                                        \
>>> +        libxl_device_ ## devtype ## _copy(ctx,(ptr),&(dev));            \
>>> +                                                                        \
>>> +        if (libxl_device_ ## devtype ## _add(ctx,(domid),&(dev), 0)) {  \
>>> +            fprintf(stderr, "libxl_device_%s_add failed.\n", #devtype); \
>>> +            exit(1);                                                    \
>>> +        }                                                               \
>>> +                                                                        \
>>> +        maybe_fixup;                                                    \
>>> +                                                                        \
>>> +        store_domain_config((domid), (d_config));                       \
>>> +        libxl_domain_config_dispose((d_config));                        \
>>> +    } while (0)
>>> +
>>
>> Is there a better way of doing this without these obtuse macros?
>>
> 
> Do you not like macro? Or do you not like this be done in xl?

I don't like macros like this.  They make the code difficult to read and
debug.

>> Is libxl lacking is a proper device abstraction layer?
> 
> libxl does have a device abstraction layer, but it cannot help much
> here. Not quite sure what "proper" means.

At a minimum it looks like a libxl__device needs copy and add methods.

This level of refactoring is probably out-of-scope of this patch series
though.

David