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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2014-07-16 at 18:12 +0100, Wei Liu wrote:
> > > As we don't have a JSON config file for libxl toolstack domain
> > > (currently Dom0) we need to skip JSON manipulation for it.
> > 
> > How hard would it be to create a stub/stunt JSON for dom0 when we notice
> > it is missing? Or from e.g. xencommons perhaps?
> > 
> 
> We need to determine:
> 1. when / where to generate such thing (xencommons?)
> 2. what to put in it
> 
> I have yet had answers for #2. The simplest version can be "{}" I think.
> That is an empty configuration that every fields gets the default value.
> But we probably need more than that.

I think you would want at least a name and perhaps a uuid? And cinfo
type == PV.

Device arrays are all empty at start of day.

Some of the stuff about target and maxmem you could perhaps infer at
start of day?

> > > +    DEVICE_ADD_JSON(vtpm, vtpms, num_vtpms, domid, &vtpm_saved,
> > 
> > The second and third of these arguments could be derived from the first.
> > 
> 
> Unfortunately not: PCI device doesn't follow this rule. Otherwise I
> would have done it already.  :-(

Ah. I had somehow concluded that you weren't using this for PCI. How
annoying.

You could consider DEVICE_ADD_JSON_SIMPLE (or some other word) which
calls the above for the non-PCI cases. Perhaps not worth it though.

> 
> > > +#define DEVICE_ADD_JSON(type, ptr, cnt, domid, dev, compare, rc)        \
> > > +    do {                                                                \
> > > +        int x;                                                          \
> > > +        libxl_domain_config d_config;                                   \
> > > +        libxl_device_##type *p;                                         \
> > > +                                                                        \
> > > +        if (domid == LIBXL_TOOLSTACK_DOMID)                             \
> > > +            break;                                                      \
> > > +                                                                        \
> > > +        libxl_domain_config_init(&d_config);                            \
> > > +                                                                        \
> > > +        rc = libxl__get_domain_configuration(gc, (domid), &d_config);   \
> > > +        if (rc)                                                         \
> > > +            goto add_done;                                              \
> > > +                                                                        \
> > > +        /* Check for duplicated device */                               \
> > > +        for (x = 0; x < d_config.cnt; x++) {                            \
> > > +            if (compare(&d_config.ptr[x], (dev))) {                     \
> > 
> > Compare just checks ->devid etc, right?
> > 
> 
> It depends on specific device type. See those COMPARE_* macros.
> Disk and PCI devices don't have devid.

Sure. I just meant to use devid as a specific example.

> > What if this device has the same devid but a different config?
> > 
> > That probably ought to be an error, have we already caught that or are
> > we relying on something later?
> > 
> 
> Then the caller should use DEVICE_UPDATE_JSON which is introduced in
> later patch.
> 
> > I suppose either way we don't want to add the device again now.
> > 
> 
> Yes.
> 
> > > +                rc = 0;                                                 \
> > > +                goto add_done;                                          \
> > > +            }                                                           \
> > > +        }                                                               \
> > > +                                                                        \
> > > +        d_config.ptr =                                                  \
> > > +            libxl__realloc(gc, d_config.ptr,                            \
> > > +                           (d_config.cnt + 1) *                         \
> > > +                           sizeof(libxl_device_##type));                \
> > > +        p = &d_config.ptr[d_config.cnt];                                \
> > > +        d_config.cnt++;                                                 \
> > > +        libxl_device_##type##_copy(CTX, p, (dev));                      \
> > > +                                                                        \
> > > +        rc = libxl__set_domain_configuration(gc, (domid), &d_config);   \
> > 
> > How close to being possible is it to do this as a proper helper function
> > which takes a size_t and a bunch of fn pointers with void where the type
> > would be?
> > 
> 
> We need to know the type of this structure otherwise we don't know what
> *_copy function to call. Sadly there's no way to pass in type information
> in C in runtime.

You could pass a copyfn as a function pointer with a void * argument for
the object to a helper function which doesn't need the specifics and
then have a macro which simply defines the type safe wrappers around
that. ISTR thinking  that the helper would need a sizeof passing to it
as well for the realloc.

Ian.


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