|
[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
Wei Liu writes ("[PATCH v1 05/10] libxl: synchronise configuration when we
hotplug a device"):
> 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.
Thanks. As I said I'm rather thick-headed today so please forgive me
if what I'm saying makes no sense. But here goes:
> @@ -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.
> + */
> + 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.
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.
> + 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;
> }
> }
>
> + if (strcmp(disk_saved.vdev, disk->vdev)) {
> + free(disk_saved.vdev);
> + disk_saved.vdev = libxl__strdup(NOGC, disk->vdev);
> + }
> +
> + DEVICE_ADD_JSON(disk, disks, num_disks, domid, &disk_saved,
> + COMPARE_DISK, rc);
> + if (rc) {
> + UNLOCK_DOMAIN_JSON_CONFIG(gc, domid, lock);
> + goto out;
> + }
> +
> rc = libxl__device_disk_setdefault(gc, disk);
> - if (rc) goto out;
> + if (rc) {
> + UNLOCK_DOMAIN_JSON_CONFIG(gc, domid, lock);
> + goto out;
> + }
>
> front = flexarray_make(gc, 16, 1);
> back = flexarray_make(gc, 16, 1);
> @@ -2217,6 +2276,7 @@ static void device_disk_add(libxl__egc *egc, uint32_t
> domid,
> if (rc != 0) {
> LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Invalid or unsupported"
> " virtual disk identifier %s", disk->vdev);
> + UNLOCK_DOMAIN_JSON_CONFIG(gc, domid, lock);
> goto out;
> }
>
> @@ -2257,6 +2317,7 @@ static void device_disk_add(libxl__egc *egc, uint32_t
> domid,
> if (!dev) {
> LOG(ERROR, "failed to get blktap devpath for %p\n",
> disk->pdev_path);
> + UNLOCK_DOMAIN_JSON_CONFIG(gc, domid, lock);
> rc = ERROR_FAIL;
> goto out;
> }
> @@ -2281,6 +2342,7 @@ static void device_disk_add(libxl__egc *egc, uint32_t
> domid,
> default:
> LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend
> type: %d\n", disk->backend);
> rc = ERROR_INVAL;
> + UNLOCK_DOMAIN_JSON_CONFIG(gc, domid, lock);
> goto out;
> }
>
> @@ -2343,9 +2405,14 @@ static void device_disk_add(libxl__egc *egc, uint32_t
> domid,
>
> rc = libxl__xs_transaction_commit(gc, &t);
> if (!rc) break;
> - if (rc < 0) goto out;
> + if (rc < 0) {
> + UNLOCK_DOMAIN_JSON_CONFIG(gc, domid, lock);
> + goto out;
> + }
> }
>
> + UNLOCK_DOMAIN_JSON_CONFIG(gc, domid, lock);
> +
> aodev->dev = device;
> aodev->action = LIBXL__DEVICE_ACTION_ADD;
> libxl__wait_device_connection(egc, aodev);
> @@ -2353,6 +2420,7 @@ static void device_disk_add(libxl__egc *egc, uint32_t
> domid,
> rc = 0;
>
> out:
> + libxl_device_disk_dispose(&disk_saved);
> libxl__xs_transaction_abort(gc, &t);
> aodev->rc = rc;
> if (rc) aodev->callback(egc, aodev);
> @@ -3009,6 +3077,11 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t
> domid,
> flexarray_t *back;
> libxl__device *device;
> unsigned int rc;
> + int lock = -1;
> + libxl_device_nic nic_saved;
> +
> + libxl_device_nic_init(&nic_saved);
> + libxl_device_nic_copy(CTX, &nic_saved, nic);
>
> rc = libxl__device_nic_setdefault(gc, nic, domid);
> if (rc) goto out;
> @@ -3023,6 +3096,9 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t
> domid,
> }
> }
>
> + nic_saved.devid = nic->devid;
> + libxl__update_config_nic(gc, &nic_saved, nic);
> +
> GCNEW(device);
> rc = libxl__device_from_nic(gc, domid, nic, device);
> if ( rc != 0 ) goto out;
> @@ -3079,17 +3155,31 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t
> domid,
> flexarray_append(front, "mac");
> flexarray_append(front, libxl__sprintf(gc,
> LIBXL_MAC_FMT,
> LIBXL_MAC_BYTES(nic->mac)));
> +
> + LOCK_DOMAIN_JSON_CONFIG(gc, domid, lock, rc);
> + if (rc) goto out;
> +
> + DEVICE_ADD_JSON(nic, nics, num_nics, domid, nic, COMPARE_DEVID, rc);
> +
> + if (rc) {
> + UNLOCK_DOMAIN_JSON_CONFIG(gc, domid, lock);
> + goto out;
> + }
> +
> libxl__device_generic_add(gc, XBT_NULL, device,
> libxl__xs_kvs_of_flexarray(gc, back,
> back->count),
> libxl__xs_kvs_of_flexarray(gc, front,
> front->count),
> NULL);
>
> + UNLOCK_DOMAIN_JSON_CONFIG(gc, domid, lock);
> +
> aodev->dev = device;
> aodev->action = LIBXL__DEVICE_ACTION_ADD;
> libxl__wait_device_connection(egc, aodev);
>
> rc = 0;
> out:
> + libxl_device_nic_dispose(&nic_saved);
> aodev->rc = rc;
> if (rc) aodev->callback(egc, aodev);
> 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 ?
Even if not, I don't think a macro is warranted here. You could make
it an inline function which returns rc.
> +#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.
> + 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
:-/.
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').
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;
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.
Thanks,
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |