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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, May 06, 2014 at 04:55:33PM +0100, Ian Campbell wrote:
> 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
> 

Many times. :-)

> > +    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?
> 

Yes. And it's not just concern, I've seen this already!
The current xl block-attach doesn't complain if you add same CD images
twice.

> > +                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).
> 

Oh I missed 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.
> 

This can be done. Will use this approach.

> > +                                                                        \
> > +        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.
> 

This is a bit different because we don't need to fix up when removing a
device. And with your approach this deep copy is not necessary.

Wei.

> Ian.

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