Re: [Xen-devel] [PATCH v1 05/10] libxl: synchronise configuration when we hotplug a device

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Fri, Jul 25, 2014 at 05:06:11PM +0100, Ian Jackson wrote:
[...]
> > +    libxl_device_disk_copy(CTX, &disk_saved, disk);
> > +
> > +    LOCK_DOMAIN_JSON_CONFIG(gc, domid, lock, rc);
> > +    if (rc) goto out;
> > +
> >      for (;;) {
> >          rc = libxl__xs_transaction_start(gc, &t);
> > -        if (rc) goto out;
> > +        if (rc) {
> > +            UNLOCK_DOMAIN_JSON_CONFIG(gc, domid, lock);
> > +            goto out;
> > +        }
> > +
> > +        /* This is a loop, if disk has been added to JSON before, we
> > +         * need to remove it, then add it later, because disk->vdev
> > +         * might change.
> > +         */
> > +        DEVICE_REMOVE_JSON(disk, disks, num_disks, domid, &disk_saved,
> > +                           COMPARE_DISK, rc);
> 
> I don't think this is right.  At this point it may be the case that
> the disk is already attached.
> 

What do you mean by "attched"? I don't think guest knows anything about
that disk at this point.

> I see in libxl__device_generic_add that we seem to unconditionally
> remove the device if we do an add of a device that's already there.  I
> think this is probably wrong.
> 
> But whether it is, or not, it is not correct to remove the entry from
> the json while it might be in xenstore.  I think you may need to do a
> xenstore read here to check and take appropriate action if it's there.
> 

I'm a bit confused. If we ever come back to the beginning of the loop,
doesn't that mean this xenstore transaction is not committed, so that
any change to xenstore entries is not visible to others? That is,
libxl__device_generic_add is doing the right thing (at least in this
loop), as well as my call to DEVICE_REMOVE_JSON.

> > +        if (rc) {
> > +            UNLOCK_DOMAIN_JSON_CONFIG(gc, domid, lock);
> > +            goto out;
> > +        }
> >  
> >          if (get_vdev) {
> >              assert(get_vdev_user);
> >              disk->vdev = get_vdev(gc, get_vdev_user, t);
> >              if (disk->vdev == NULL) {
> >                  rc = ERROR_FAIL;
> > +                UNLOCK_DOMAIN_JSON_CONFIG(gc, domid, lock);
> >                  goto out;
> >              }
> >          }
> >  
[...]
> >      return;
> > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> > index 72d21ad..f27a6e98 100644
> > --- a/tools/libxl/libxl_internal.h
> > +++ b/tools/libxl/libxl_internal.h
> > @@ -3254,6 +3254,105 @@ static inline void 
> > libxl__update_config_vtpm(libxl__gc *gc,
> >      libxl_uuid_copy(CTX, &dst->uuid, &src->uuid);
> >  }
> >  
> > +/* Macro used to add the new device to JSON template */
> > +#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)->func == (b)->func &&    \
> > +                           (a)->bus == (b)->bus &&      \
> > +                           (a)->dev == (b)->dev)
> > +
> > +#define LOCK_DOMAIN_JSON_CONFIG(gc, domid, lock, rc)                 \
> > +    do {                                                             \
> > +        if (domid != LIBXL_TOOLSTACK_DOMID)                          \
> > +            rc = libxl__lock_domain_configuration(gc, domid, &lock); \
> > +        else                                                         \
> > +            rc = 0;                                                  \
> > +    } while (0)
> 
> Can this test be perhaps moved into libxl__lock_domain_configuration ?
> 

Sure, but Ian C suggested I generate a stub JSON config for Dom0 so that
we can get rid of this special case. I think I will go down that route.

> Even if not, I don't think a macro is warranted here.  You could make
> it an inline function which returns rc.
> 

No problem.

> > +#define DEVICE_ADD_JSON(type, ptr, cnt, domid, dev, compare, rc)        \
> 
> This needs a good doc comment.  See the doc comments for things like
> GCSPRINTF, CONTAINER_OF, CTYPE, etc., for the kind of thing that's
> helpful.
> 

Ack.

> > +    do {                                                                \
> > +        int x;                                                          \
> > +        libxl_domain_config d_config;                                   \
> > +        libxl_device_##type *p;                                         \
> 
> We have -Wshadow so you need to namespace all of these variable names
> :-/.
> 

Yes I know, and build system has not complained so far, but ...

> That is, give them names which are such that surrounding code won't
> clash with.  Presumably this is the reason why you have `int x' rather
> than `int i' but that's not sufficient because in general the macro's
> user might have an `int x'.  I suggest something like
>   +        int DAJ_x;                                                    \
>   +        libxl_domain_config DAJ_cfg;                                  \
>   +        libxl_device_##type *DAJ_dev;                                 \
> or the like.
> 
> The same goes for the `add_done' label.  It should probably be
> `DAJ_out' (since it's a kind of `out').
> 

I think I will use this approach in my next version.

> 
> You can get rid of the rc macro parameter if you:
>  - replace the do{ } encapsulation with a gcc block expression ({ })
>  - make rc a namespaced local variable eg
>        ({              \
>            int DAJ_rc; \
>  - end the ({ }) expression like this
>            DAJ_rc;   \
>        )}
> This would mean that the macro wouldn't set the caller's rc; it
> would instead return it like our other macros.  The user would write
>     rc = DEVICE_ADD_JSON(type, ptr, cnt, domid, dev, compare);
>     if (rc) goto out;
> 

Ack.

> 
> I considered whether it would be possible to make this macro a
> function instead but I think it's quite tricky because it would
> involve a lot of casting.  (Eg of a function pointer type.)  I haven't
> looked up the exact rules to see whether the resulting code would be
> legal C99 but I think it would probably be clumsier so we should
> probably stick with the macro.
> 

Yes I would rather stick with this macro.

Wei.

> 
> Thanks,
> Ian.

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