Re: [Xen-devel] [PATCH V4 24/24] xl: update configuration when we unplug a device

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2014-05-01 at 13:58 +0100, Wei Liu wrote:
> Some macros are written to accomplish following tasks:
> 1. load domain configuration
> 2. allocate a new array of devices
> 3. copy the devices that are to be remain in configuration

s/are to be// and s/in/into/.

> 4. replace pointer in libxl_domain_config
> 5. store domain configuration
> 
> Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> ---
>  tools/libxl/xl_cmdimpl.c |   83 
> ++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 69 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 91172c5..ae3df6e 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -411,6 +411,59 @@ static void *xrealloc(void *ptr, size_t sz) {
>          libxl_domain_config_dispose((d_config));                        \
>      } while (0)
>  
> +#define COMPARE_DEVID(a, b) ((a)->devid == (b)->devid)
> +#define COMPARE_DISK(a, b) (!strcmp((a)->vdev, (b)->vdev))
> +#define COMPARE_PCI(a, b) ((a)->bus == (b)->bus &&      \
> +                               (a)->dev == (b)->dev &&  \
> +                           (a)->func == (b)->func)
> +
> +/* Remove / destroy a device and update the stored configuration */
> +#define REMOVEDESTROY_DEVICE(devtype,domid,dev,ptr,cnt,                 \
> +                             compare,removedestroy)                     \

You've been talking to Ian J haven't you :-P

> +    do {                                                                \
> +        libxl_domain_config d_config;                                   \
> +        libxl_device_ ## devtype *p = NULL, *x;                         \
> +        int num;                                                        \
> +        int j, k;                                                       \
> +                                                                        \
> +        libxl_domain_config_init(&d_config);                            \
> +        load_domain_config((domid), &d_config);                         \
> +                                                                        \
> +        k = 0;                                                          \
> +        for (j = 0; j < d_config.cnt; j++) {                            \
> +            x = d_config.ptr + j;                                       \
> +            if (compare(x, &(dev)))                                     \

Are you concerned with the possibility that two entries in the array
might match dev? Wouldn't that equate to e.g. two xvda devices? Should
we not reject such things elsewhere?

> +                k++;                                                    \
> +        }                                                               \
> +                                                                        \
> +        num = d_config.cnt - k;                                         \
> +                                                                        \
> +        p = xrealloc(p, sizeof(*p) * num);                              \

Why realloc, isn't the input p always NULL? (turning it into xmalloc).

But couldn't this be done in place, with a little care?

        for(i = j = 0; i < d_config.cnt; i++)
            if (!compare(d_config.ptr[i], dev)
                d_config.ptr[j] = d_config.ptr[i] /* XXX optimise/skip if j == 
i */
                j++
(note that j <= i is always true)

then realloc the array down to only j entries.

> +                                                                        \
> +        k = 0;                                                          \
> +        for (j = 0; j < d_config.cnt; j++) {                            \
> +            x = d_config.ptr + j;                                       \
> +            if (compare(x, &(dev)))                                     \
> +                continue;                                               \
> +            libxl_device_ ## devtype ## _copy(ctx, &p[k],               \
> +                                              d_config.ptr + j);        \

Same comment as the previous patch re the actual need for a deep copy.

Ian.


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