[PATCH v4 for 4.5 2/8] libxl: synchronise configuration when we hotplug a device

[Wei Liu <wei.liu2@citrix.com>]

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 workflow is as followed:
   lock json config
       read json config
       update in-memory json config with new entry, replacing
         any stale entry
       for loop
           open xs transaction
           check device existence, abort if it exists
           write in-memory json config to disk
           commit xs transaction
       end for loop
   unlock json config

Please see comment in libxl_internal.h for correctness proof.

As those routines are called both during domain creation and device
hotplug, we add a flag to indicate whether we need to update JSON
config. This flag is only set to true when we hotplug a device. We
cannot update JSON config during domain creation as JSON config is
committed to disk only when domain creation finishes.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
change in v4:
1. correct typos and reword comment
2. libxl__carefd -> libxl__domain_userdata_lock
---
 tools/libxl/libxl.c          |   96 +++++++++++++++++++++++++++++++
 tools/libxl/libxl_internal.h |  130 ++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_pci.c      |   24 ++++++++
 3 files changed, 250 insertions(+)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 3b2d104..e5d4891 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1894,6 +1894,13 @@ void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid,
     libxl__device *device;
     int rc;
     xs_transaction_t t = XBT_NULL;
+    libxl_domain_config d_config;
+    libxl_device_vtpm vtpm_saved;
+    libxl__domain_userdata_lock *lock = NULL;
+
+    libxl_domain_config_init(&d_config);
+    libxl_device_vtpm_init(&vtpm_saved);
+    libxl_device_vtpm_copy(CTX, &vtpm_saved, vtpm);
 
     rc = libxl__device_vtpm_setdefault(gc, vtpm);
     if (rc) goto out;
@@ -1908,6 +1915,8 @@ void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid,
         }
     }
 
+    libxl__update_config_vtpm(gc, &vtpm_saved, vtpm);
+
     GCNEW(device);
     rc = libxl__device_from_vtpm(gc, domid, vtpm, device);
     if ( rc != 0 ) goto out;
@@ -1933,6 +1942,19 @@ void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid,
     flexarray_append(front, "handle");
     flexarray_append(front, GCSPRINTF("%d", vtpm->devid));
 
+    if (aodev->update_json) {
+        lock = libxl__lock_domain_userdata(gc, domid);
+        if (!lock) {
+            rc = ERROR_LOCK_FAIL;
+            goto out;
+        }
+
+        rc = libxl__get_domain_configuration(gc, domid, &d_config);
+        if (rc) goto out;
+
+        DEVICE_ADD(vtpm, vtpms, domid, &vtpm_saved, COMPARE_DEVID, &d_config);
+    }
+
     for (;;) {
         rc = libxl__xs_transaction_start(gc, &t);
         if (rc) goto out;
@@ -1946,6 +1968,11 @@ void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid,
             goto out;
         }
 
+        if (aodev->update_json) {
+            rc = libxl__set_domain_configuration(gc, domid, &d_config);
+            if (rc) goto out;
+        }
+
         libxl__device_generic_add(gc, t, device,
                                   libxl__xs_kvs_of_flexarray(gc, back,
                                                              back->count),
@@ -1965,6 +1992,9 @@ void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid,
     rc = 0;
 out:
     libxl__xs_transaction_abort(gc, &t);
+    if (lock) libxl__unlock_domain_userdata(lock);
+    libxl_device_vtpm_dispose(&vtpm_saved);
+    libxl_domain_config_dispose(&d_config);
     aodev->rc = rc;
     if(rc) aodev->callback(egc, aodev);
     return;
@@ -2197,6 +2227,13 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
     int rc;
     libxl_ctx *ctx = gc->owner;
     xs_transaction_t t = XBT_NULL;
+    libxl_domain_config d_config;
+    libxl_device_disk disk_saved;
+    libxl__domain_userdata_lock *lock = NULL;
+
+    libxl_domain_config_init(&d_config);
+    libxl_device_disk_init(&disk_saved);
+    libxl_device_disk_copy(ctx, &disk_saved, disk);
 
     libxl_domain_type type = libxl__domain_type(gc, domid);
     if (type == LIBXL_DOMAIN_TYPE_INVALID) {
@@ -2204,6 +2241,26 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
         goto out;
     }
 
+    /*
+     * get_vdev != NULL -> local attach
+     * get_vdev == NULL -> block attach
+     *
+     * We don't care about local attach state because it's only
+     * intermediate state.
+     */
+    if (!get_vdev && aodev->update_json) {
+        lock = libxl__lock_domain_userdata(gc, domid);
+        if (!lock) {
+            rc = ERROR_LOCK_FAIL;
+            goto out;
+        }
+
+        rc = libxl__get_domain_configuration(gc, domid, &d_config);
+        if (rc) goto out;
+
+        DEVICE_ADD(disk, disks, domid, &disk_saved, COMPARE_DISK, &d_config);
+    }
+
     for (;;) {
         rc = libxl__xs_transaction_start(gc, &t);
         if (rc) goto out;
@@ -2356,6 +2413,11 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
             }
         }
 
+        if (!get_vdev && aodev->update_json) {
+            rc = libxl__set_domain_configuration(gc, domid, &d_config);
+            if (rc) goto out;
+        }
+
         libxl__device_generic_add(gc, t, device,
                                   libxl__xs_kvs_of_flexarray(gc, back, back->count),
                                   libxl__xs_kvs_of_flexarray(gc, front, front->count),
@@ -2374,6 +2436,9 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
 
 out:
     libxl__xs_transaction_abort(gc, &t);
+    if (lock) libxl__unlock_domain_userdata(lock);
+    libxl_device_disk_dispose(&disk_saved);
+    libxl_domain_config_dispose(&d_config);
     aodev->rc = rc;
     if (rc) aodev->callback(egc, aodev);
     return;
@@ -3030,6 +3095,13 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t domid,
     libxl__device *device;
     int rc;
     xs_transaction_t t = XBT_NULL;
+    libxl_domain_config d_config;
+    libxl_device_nic nic_saved;
+    libxl__domain_userdata_lock *lock = NULL;
+
+    libxl_domain_config_init(&d_config);
+    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;
@@ -3044,6 +3116,8 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t domid,
         }
     }
 
+    libxl__update_config_nic(gc, &nic_saved, nic);
+
     GCNEW(device);
     rc = libxl__device_from_nic(gc, domid, nic, device);
     if ( rc != 0 ) goto out;
@@ -3101,6 +3175,19 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t domid,
     flexarray_append(front, libxl__sprintf(gc,
                                     LIBXL_MAC_FMT, LIBXL_MAC_BYTES(nic->mac)));
 
+    if (aodev->update_json) {
+        lock = libxl__lock_domain_userdata(gc, domid);
+        if (!lock) {
+            rc = ERROR_LOCK_FAIL;
+            goto out;
+        }
+
+        rc = libxl__get_domain_configuration(gc, domid, &d_config);
+        if (rc) goto out;
+
+        DEVICE_ADD(nic, nics, domid, &nic_saved, COMPARE_DEVID, &d_config);
+    }
+
     for (;;) {
         rc = libxl__xs_transaction_start(gc, &t);
         if (rc) goto out;
@@ -3114,6 +3201,11 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t domid,
             goto out;
         }
 
+        if (aodev->update_json) {
+            rc = libxl__set_domain_configuration(gc, domid, &d_config);
+            if (rc) goto out;
+        }
+
         libxl__device_generic_add(gc, t, device,
                                   libxl__xs_kvs_of_flexarray(gc, back,
                                                              back->count),
@@ -3133,6 +3225,9 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t domid,
     rc = 0;
 out:
     libxl__xs_transaction_abort(gc, &t);
+    if (lock) libxl__unlock_domain_userdata(lock);
+    libxl_device_nic_dispose(&nic_saved);
+    libxl_domain_config_dispose(&d_config);
     aodev->rc = rc;
     if (rc) aodev->callback(egc, aodev);
     return;
@@ -3686,6 +3781,7 @@ DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
         GCNEW(aodev);                                                   \
         libxl__prepare_ao_device(ao, aodev);                            \
         aodev->callback = device_addrm_aocomplete;                      \
+        aodev->update_json = true;                                      \
         libxl__device_##type##_add(egc, domid, type, aodev);            \
                                                                         \
         return AO_INPROGRESS;                                           \
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index aa18e74..f61673c 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2148,6 +2148,8 @@ struct libxl__ao_device {
     int num_exec;
     /* for calling hotplug scripts */
     libxl__async_exec_state aes;
+    /* If we need to update JSON config */
+    bool update_json;
 };
 
 /*
@@ -2273,6 +2275,77 @@ struct libxl__multidev {
  *
  * Once finished, aodev->callback will be executed.
  */
+/*
+ * As of Xen 4.5 we maintain various infomation, including hotplug
+ * device information, in JSON files, so that we can use this JSON
+ * file as a template to reconstruct domain configuration.
+ *
+ * In essense there are now two views of device state, one is xenstore,
+ * the other is JSON file. We use xenstore as primary reference.
+ *
+ * Here we maintain one invariant: every device in xenstore must have
+ * an entry in JSON file.
+ *
+ * All device hotplug routines should comply to following pattern:
+ *   lock json config (json_lock)
+ *       read json config
+ *       update in-memory json config with new entry, replacing
+ *          any stale entry
+ *       for loop -- xs transaction
+ *           open xs transaction
+ *           check device existence, abort if it exists
+ *           write in-memory json config to disk
+ *           commit xs transaction
+ *       end for loop
+ *   unlock json config
+ *
+ * Device removal routines are not touched.
+ *
+ * Here is the proof that we always maintain that invariant and we
+ * don't leak files during interaction of hotplug thread and other
+ * threads / processes.
+ *
+ * # Safe against parallel add
+ *
+ * When another thread / process tries to add same device, it's
+ * blocked by json_lock. The loser of two threads will bail at
+ * existence check, so that we don't overwrite anything.
+ *
+ * # Safe against domain destruction
+ *
+ * If the thread / process trying to destroy domain loses the race, it's
+ * blocked by json_lock. If the hotplug thread is loser, it bails at
+ * acquiring lock because lock acquisition function checks existence of
+ * the domain.
+ *
+ * # Safe against parallel removal
+ *
+ * When another thread / process tries to remove a device, it's _NOT_
+ * blocked by json_lock, but xenstore transaction can help maintain
+ * invariant. The removal threads either a) sees that device in
+ * xenstore, b) doesn't see that device in xenstore.
+ *
+ * In a), it sees that device in xenstore. At that point hotplug is
+ * already finished (both JSON and xenstore changes committed). So that
+ * device can be safely removed. JSON entry is left untouched and
+ * becomes stale, but this is a valid state -- next time when a
+ * device with same identifier gets added, the stale entry gets
+ * overwritten.
+ *
+ * In b), it doesn't see that device in xenstore, but it will commence
+ * anyway. Eventually a forcibly removal is initiated, which will forcely
+ * remove xenstore entry.
+ *
+ * If hotplug threads creates xenstore entry (therefore JSON entry as
+ * well) before force removal, that xenstore entry is removed. We're
+ * left with JSON stale entry but not xenstore entry, which is a valid
+ * state.
+ *
+ * If hotplug thread has not created xenstore entry when the removal
+ * is committed, we're obviously safe. Hotplug thread will add in
+ * xenstore entry afterwards. We have both JSON and xenstore entry,
+ * it's a valid state.
+ */
 _hidden void libxl__device_disk_add(libxl__egc *egc, uint32_t domid,
                                     libxl_device_disk *disk,
                                     libxl__ao_device *aodev);
@@ -3280,6 +3353,63 @@ static inline void libxl__update_config_vtpm(libxl__gc *gc,
     libxl_uuid_copy(CTX, &dst->uuid, &src->uuid);
 }
 
+/* Macros used to compare device identifier. Returns true if the two
+ * devices have same identifier. */
+#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)
+
+/* DEVICE_ADD
+ *
+ * Add a device in libxl_domain_config structure
+ *
+ * It takes 6 parameters:
+ *  type:     the type of the device, say nic, vtpm, disk, pci etc
+ *  ptr:      pointer to the start of the array, the array must be
+ *            of type libxl_device_#type
+ *  domid:    domain id of target domain
+ *  dev:      the device that is to be added / removed / updated
+ *  compare:  the COMPARE_* macro used to compare @dev's identifier to
+ *            those in the array pointed to by @ptr
+ *  d_config: pointer to template domain config
+ *
+ * For most device types (nic, vtpm), the array pointer @ptr can be
+ * derived from @type, pci device being the exception, hence we need
+ * to have @ptr.
+ *
+ * If there is already a device with the same identifier in d_config,
+ * that entry is updated.
+ */
+#define DEVICE_ADD(type, ptr, domid, dev, compare, d_config)    \
+    ({                                                          \
+        int DA_x;                                               \
+        libxl_device_##type *DA_p = NULL;                       \
+                                                                \
+        /* Check for existing device */                         \
+        for (DA_x = 0; DA_x < (d_config)->num_##ptr; DA_x++) {  \
+            if (compare(&(d_config)->ptr[DA_x], (dev))) {       \
+                DA_p = &(d_config)->ptr[DA_x];                  \
+                break;                                          \
+            }                                                   \
+        }                                                       \
+                                                                \
+        if (!DA_p) {                                            \
+            (d_config)->ptr =                                   \
+                libxl__realloc(NOGC, (d_config)->ptr,           \
+                               ((d_config)->num_##ptr + 1) *    \
+                               sizeof(libxl_device_##type));    \
+            DA_p = &(d_config)->ptr[(d_config)->num_##ptr];     \
+            (d_config)->num_##ptr++;                            \
+        } else {                                                \
+            libxl_device_##type##_dispose(DA_p);                \
+        }                                                       \
+                                                                \
+        libxl_device_##type##_init(DA_p);                       \
+        libxl_device_##type##_copy(CTX, DA_p, (dev));           \
+    })
+
 #endif
 
 /*
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index c22767e..9f40100 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -126,6 +126,13 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, uint32_t domid, libxl_d
     xs_transaction_t t;
     libxl__device *device;
     int rc;
+    libxl_domain_config d_config;
+    libxl_device_pci pcidev_saved;
+    libxl__domain_userdata_lock *lock = NULL;
+
+    libxl_domain_config_init(&d_config);
+    libxl_device_pci_init(&pcidev_saved);
+    libxl_device_pci_copy(CTX, &pcidev_saved, pcidev);
 
     be_path = libxl__sprintf(gc, "%s/backend/pci/%d/0", libxl__xs_get_dompath(gc, 0), domid);
     num_devs = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/num_devs", be_path));
@@ -153,6 +160,17 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, uint32_t domid, libxl_d
     GCNEW(device);
     libxl__device_from_pcidev(gc, domid, pcidev, device);
 
+    lock = libxl__lock_domain_userdata(gc, domid);
+    if (!lock) {
+        rc = ERROR_LOCK_FAIL;
+        goto out;
+    }
+
+    rc = libxl__get_domain_configuration(gc, domid, &d_config);
+    if (rc) goto out;
+
+    DEVICE_ADD(pci, pcidevs, domid, &pcidev_saved, COMPARE_PCI, &d_config);
+
     for (;;) {
         rc = libxl__xs_transaction_start(gc, &t);
         if (rc) goto out;
@@ -165,6 +183,9 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, uint32_t domid, libxl_d
             goto out;
         }
 
+        rc = libxl__set_domain_configuration(gc, domid, &d_config);
+        if (rc) goto out;
+
         libxl__xs_writev(gc, t, be_path,
                          libxl__xs_kvs_of_flexarray(gc, back, back->count));
 
@@ -175,6 +196,9 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, uint32_t domid, libxl_d
 
 out:
     libxl__xs_transaction_abort(gc, &t);
+    if (lock) libxl__unlock_domain_userdata(lock);
+    libxl_device_pci_dispose(&pcidev_saved);
+    libxl_domain_config_dispose(&d_config);
     return rc;
 }
 
-- 
1.7.10.4