|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v1 05/10] libxl: synchronise configuration when we hotplug a device
On Thu, 2014-07-10 at 15:32 +0100, Wei Liu wrote:
> We update JSON version first, then write to xenstore, so that we
> maintain the following invariant: any device which is present in
> xenstore has a corresponding entry in JSON.
>
> The locking pattern is as followed:
> 1. lock JSON domain config
> 2. write JSON entry
> 3. write xenstore entry for a device
> 4. unlock JSON domain config
>
> And we ensure in code that JSON entry is always written before the
> xenstore entry is written. That is, if 2 fails, 3 will not be executed.
> We also ensure that if 2 and 3 are invoked in a loop, the previous
> added JSON entry is removed before writing new one.
>
> The locking patten is designed like this, because when adding a disk,
"pattern". Apparently a patten is a type of clog or shoe l-)
> the caller might specify get_vdev which will get called in the middle
> of a xenstore transaction to determine the identifier of a disk. Other
> devices don't have this property, but I make them follow the same
> pattern for consistency.
I don't know if it is helpful but AIUI the get_vdev there is purely to
handle the case where the disk is being attached to the toolstack domain
(e.g. for pygrub). I don't know if that simplifies anything for you.
> 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?
> Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> @@ -1917,6 +1922,8 @@ void libxl__device_vtpm_add(libxl__egc *egc, uint32_t
> domid,
> goto out;
> }
> }
> + vtpm_saved.devid = vtpm->devid;
Why isn't this in the update_config fn?
> + libxl__update_config_vtpm(gc, &vtpm_saved, vtpm);
>
> GCNEW(device);
> rc = libxl__device_from_vtpm(gc, domid, vtpm, device);
> @@ -1943,17 +1950,30 @@ void libxl__device_vtpm_add(libxl__egc *egc, uint32_t
> domid,
> flexarray_append(front, "handle");
> flexarray_append(front, GCSPRINTF("%d", vtpm->devid));
>
> + LOCK_DOMAIN_JSON_CONFIG(gc, domid, lock, rc);
> + if (rc) goto out;
I can't see anything in this macro (or the unlock one) which
necessitates it being a macro rather than a helper function.
Couldn't you just do the LIBXL_TOOLSTACK_DOMID check in
libxl__lock_domain_configuration?
> +
> + DEVICE_ADD_JSON(vtpm, vtpms, num_vtpms, domid, &vtpm_saved,
The second and third of these arguments could be derived from the first.
> @@ -2193,21 +2217,56 @@ static void device_disk_add(libxl__egc *egc, uint32_t
> domid,
> goto out;
> }
>
> + 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.
Has the old device been removed from xenstore at this point? Otherwise
aren't you breaking your invariant that a device in xenstore is always
described in the json.
> + */
> + DEVICE_REMOVE_JSON(disk, disks, num_disks, domid, &disk_saved,
> + COMPARE_DISK, rc);
> + if (rc) {
> + UNLOCK_DOMAIN_JSON_CONFIG(gc, domid, lock);
> + goto out;
I wonder if you can arrange the exit path such that this can happen
there and save all of these?
> +#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?
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?
I suppose either way we don't want to add the device again now.
> + 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?
> + \
> + add_done: \
You are going to have multiple of these labels in this source file. Not
sure quite what that means in C. The simple answer would be to involve
##type## in the name.
> + libxl_domain_config_dispose(&d_config); \
> + } while (0)
> +
> +#define DEVICE_REMOVE_JSON(type, ptr, cnt, domid, dev, compare, rc) \
Same comments as on add.
> + do { \
> + int i, j; \
> + libxl_device_##type *p = dev; \
> + libxl_domain_config d_config; \
> + bool removed = false; \
> + \
> + if (domid == LIBXL_TOOLSTACK_DOMID) \
> + break; \
> + \
> + libxl_domain_config_init(&d_config); \
> + \
> + rc = libxl__get_domain_configuration(gc, (domid), &d_config); \
> + if (rc) \
> + goto remove_done; \
> + \
> + for (i = j = 0; i < d_config.cnt; i++) { \
> + if (!compare(&d_config.ptr[i], p)) { \
> + if (i != j) { \
> + libxl_device_##type##_dispose(&d_config.ptr[j]); \
> + d_config.ptr[j] = d_config.ptr[i]; \
> + removed = true; \
> + } \
> + j++; \
> + } \
> + } \
> + \
> + if (!removed) /* no matching entry found */ \
> + goto remove_done; \
> + \
> + d_config.ptr = \
> + libxl__realloc(NOGC, d_config.ptr, \
> + j * sizeof(libxl_device_##type)); \
> + d_config.cnt = j; \
The fact that the compare loop also shuffles everything down is worthy
of noting in a comment I think.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |