[PATCH v4 for 4.5 0/8] libxl: synchronise domain configuration

[PATCH v4 for 4.5 0/8] libxl: synchronise domain configuration

[Wei Liu]

Version 4 of this series based on master branch.  This series can be pulled
from:

 git://xenbits.xen.org/people/liuw/xen.git wip.config-sync4

Delta from v3 is listed in individual commit log.

This version has passed following tests:
  test-amd64-amd64-xl
  test-amd64-amd64-xl-qemuu-debianhvm-amd64
  test-amd64-amd64-xl-qemut-debianhvm-amd64

Wei.

Legend: A - Acked, N - New patch in this round

Wei Liu (8):
 N  libxl: rework domain userdata file lock
    libxl: synchronise configuration when we hotplug a device
    libxl: make libxl_cd_insert "eject" + "insert"
    libxl: refactor libxl_get_memory_target
    libxl: introduce libxl_retrieve_domain_configuration
 A  libxl: introduce libxl_userdata_unlink
 A  xl: use libxl_retrieve_domain_configuration and JSON format
 A  xl: long output of "list" command now contains Dom0 information

 docs/man/xl.pod.1            |    5 +
 tools/libxl/libxl.c          |  489 ++++++++++++++++++++++++++++++++++++++----
 tools/libxl/libxl.h          |   26 +++
 tools/libxl/libxl_create.c   |    2 +-
 tools/libxl/libxl_dom.c      |   36 +++-
 tools/libxl/libxl_internal.c |   30 ++-
 tools/libxl/libxl_internal.h |  139 +++++++++++-
 tools/libxl/libxl_pci.c      |   24 +++
 tools/libxl/xl_cmdimpl.c     |  127 ++++++-----
 tools/libxl/xl_cmdtable.c    |    4 +-
 10 files changed, 777 insertions(+), 105 deletions(-)

-- 
1.7.10.4

[PATCH v4 for 4.5 1/8] libxl: rework domain userdata file lock

[Wei Liu]

The lock introduced in d2cd9d4f ("libxl: functions to lock / unlock
libxl userdata store") has a bug that can leak the lock file when domain
destruction races with other functions that try to get hold of the lock.

There are several issues:
1. The lock is released too early with libxl__userdata_destroyall
   deletes everything in userdata store, including the lock file.
2. The check of domain existence is only done at the beginning of lock
   function, by the time the lock is acquired, the domain might have
   been gone already.

The effect of this two issues is we can run into such situation:

     Process 1                        Process 2 domain destruction
   # LOCK FUNCTION                 # LOCK FUNCTION
    check domain existence          check domain existence
                                    acquire lock (file created)
                                   # LOCK FUNCTION
                                    destroy all files (lock file deleted,
                                                       lock released)
    acquire lock (file created)
   # LOCK FUNCTION                  destroy domain
                                   # UNLOCK (close fd only)
   [ lock file leaked ]

Fix this problem by deploying following changes:

1. Unlink lock file in unlock function.
2. Modify libxl__userdata_destroyall to not delete domain-userdata-lock,
   so that the lock remains held until unlock function is called.
3. Check domain still exists when the lock is acquired, unlock if
   domain is already gone.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl.c          |    2 +-
 tools/libxl/libxl_create.c   |    2 +-
 tools/libxl/libxl_dom.c      |   10 +++++++---
 tools/libxl/libxl_internal.c |   30 +++++++++++++++++++++---------
 tools/libxl/libxl_internal.h |    9 +++++++--
 5 files changed, 37 insertions(+), 16 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index ad3495a..3b2d104 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1530,7 +1530,7 @@ static void devices_destroy_cb(libxl__egc *egc,
     uint32_t domid = dis->domid;
     char *dom_path;
     char *vm_path;
-    libxl__carefd *lock = NULL;
+    libxl__domain_userdata_lock *lock = NULL;
 
     dom_path = libxl__xs_get_dompath(gc, domid);
     if (!dom_path) {
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index a5e185e..8b82584 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1404,7 +1404,7 @@ static void domcreate_complete(libxl__egc *egc,
         rc = xc_flask_relabel_domain(CTX->xch, dcs->guest_domid, d_config->b_info.exec_ssidref);
 
     if (!rc) {
-        libxl__carefd *lock;
+        libxl__domain_userdata_lock *lock;
 
         /* Note that we hold CTX lock at this point so only need to
          * take data store lock
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 0dfdb08..02384ac 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1857,8 +1857,12 @@ void libxl__userdata_destroyall(libxl__gc *gc, uint32_t domid)
     if (r)
         LOGE(ERROR, "glob failed for %s", pattern);
 
+    /* Note: don't delete domain-userdata-lock, it will be handled by
+     * unlock function.
+     */
     for (i=0; i<gl.gl_pathc; i++) {
-        userdata_delete(gc, gl.gl_pathv[i]);
+        if (!strstr(gl.gl_pathv[i], "domain-userdata-lock"))
+            userdata_delete(gc, gl.gl_pathv[i]);
     }
     globfree(&gl);
 out:
@@ -1931,7 +1935,7 @@ int libxl_userdata_store(libxl_ctx *ctx, uint32_t domid,
 {
     GC_INIT(ctx);
     int rc;
-    libxl__carefd *lock;
+    libxl__domain_userdata_lock *lock;
 
     CTX_LOCK;
     lock = libxl__lock_domain_userdata(gc, domid);
@@ -1992,7 +1996,7 @@ int libxl_userdata_retrieve(libxl_ctx *ctx, uint32_t domid,
 {
     GC_INIT(ctx);
     int rc;
-    libxl__carefd *lock;
+    libxl__domain_userdata_lock *lock;
 
     CTX_LOCK;
     lock = libxl__lock_domain_userdata(gc, domid);
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index e9747f1..02a71cb 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -384,9 +384,10 @@ out:
 /* Portability note: this lock utilises flock(2) so a proper implementation of
  * flock(2) is required.
  */
-libxl__carefd *libxl__lock_domain_userdata(libxl__gc *gc, uint32_t domid)
+libxl__domain_userdata_lock *libxl__lock_domain_userdata(libxl__gc *gc,
+                                                         uint32_t domid)
 {
-    libxl__carefd *carefd = NULL;
+    libxl__domain_userdata_lock *lock = NULL;
     const char *lockfile;
     int fd;
     struct stat stab, fstab;
@@ -394,12 +395,15 @@ libxl__carefd *libxl__lock_domain_userdata(libxl__gc *gc, uint32_t domid)
     lockfile = libxl__userdata_path(gc, domid, "domain-userdata-lock", "l");
     if (!lockfile) goto out;
 
+    lock = libxl__zalloc(NOGC, sizeof(libxl__domain_userdata_lock));
+    lock->path = libxl__strdup(NOGC, lockfile);
+
     while (true) {
         libxl__carefd_begin();
         fd = open(lockfile, O_RDWR|O_CREAT, 0666);
         if (fd < 0)
             LOGE(ERROR, "cannot open lockfile %s, errno=%d", lockfile, errno);
-        carefd = libxl__carefd_opened(CTX, fd);
+        lock->lock_carefd = libxl__carefd_opened(CTX, fd);
         if (fd < 0) goto out;
 
         /* Lock the file in exclusive mode, wait indefinitely to
@@ -434,20 +438,28 @@ libxl__carefd *libxl__lock_domain_userdata(libxl__gc *gc, uint32_t domid)
                 break;
         }
 
-        libxl__carefd_close(carefd);
+        libxl__carefd_close(lock->lock_carefd);
     }
 
-    return carefd;
+    /* Check the domain is still there, if not we should release the
+     * lock and clean up.
+     */
+    if (libxl_domain_info(CTX, NULL, domid))
+        goto out;
+
+    return lock;
 
 out:
-    if (carefd) libxl__carefd_close(carefd);
+    if (lock) libxl__unlock_domain_userdata(lock);
     return NULL;
 }
 
-void libxl__unlock_domain_userdata(libxl__carefd *lock_carefd)
+void libxl__unlock_domain_userdata(libxl__domain_userdata_lock *lock)
 {
-    /* Simply closing the file descriptor releases the lock */
-    libxl__carefd_close(lock_carefd);
+    if (lock->path) unlink(lock->path);
+    if (lock->lock_carefd) libxl__carefd_close(lock->lock_carefd);
+    free(lock->path);
+    free(lock);
 }
 
 int libxl__get_domain_configuration(libxl__gc *gc, uint32_t domid,
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 03e9978..aa18e74 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3241,8 +3241,13 @@ static inline int libxl__key_value_list_is_empty(libxl_key_value_list *pkvl)
 int libxl__cpuid_policy_is_empty(libxl_cpuid_policy_list *pl);
 
 /* Portability note: a proper flock(2) implementation is required */
-libxl__carefd *libxl__lock_domain_userdata(libxl__gc *gc, uint32_t domid);
-void libxl__unlock_domain_userdata(libxl__carefd *lock_carefd);
+typedef struct {
+    libxl__carefd *lock_carefd;
+    char *path; /* path of the lock file itself */
+} libxl__domain_userdata_lock;
+libxl__domain_userdata_lock *libxl__lock_domain_userdata(libxl__gc *gc,
+                                                         uint32_t domid);
+void libxl__unlock_domain_userdata(libxl__domain_userdata_lock *lock);
 
 /*
  * Retrieve / store domain configuration from / to libxl private
-- 
1.7.10.4

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

[Wei Liu]

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

[PATCH v4 for 4.5 3/8] libxl: make libxl_cd_insert "eject" + "insert"

[Wei Liu]

We introduce an intermediate empty state when inserting media into
CDROM. The scheme works like this:

  lock json config
  write empty state to xenstore
  for (;;) {
      write user supplied disk to JSON
      write disk information to xenstore
  }
  unlock json config

Bear in mind that all steps can fail. With the proposed scheme, we now
know, if xenstore is empty, then CDROM should be considered empty;
otherwise we should use JSON version of CDROM configuration.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl.c |  119 ++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 98 insertions(+), 21 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index e5d4891..89f4ce0 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2664,14 +2664,26 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
 {
     AO_CREATE(ctx, domid, ao_how);
     int num = 0, i;
-    libxl_device_disk *disks = NULL;
+    libxl_device_disk *disks = NULL, disk_saved, disk_empty;
+    libxl_domain_config d_config;
     int rc, dm_ver;
-
     libxl__device device;
     const char * path;
     char * tmp;
+    libxl__domain_userdata_lock *lock = NULL;
+    xs_transaction_t t = XBT_NULL;
+    flexarray_t *insert = NULL, *empty = NULL;
 
-    flexarray_t *insert = NULL;
+    libxl_domain_config_init(&d_config);
+    libxl_device_disk_init(&disk_empty);
+    libxl_device_disk_init(&disk_saved);
+    libxl_device_disk_copy(ctx, &disk_saved, disk);
+
+    disk_empty.format = LIBXL_DISK_FORMAT_EMPTY;
+    disk_empty.vdev = libxl__strdup(NOGC, disk->vdev);
+    disk_empty.pdev_path = libxl__strdup(NOGC, "");
+    disk_empty.is_cdrom = 1;
+    libxl__device_disk_setdefault(gc, &disk_empty);
 
     libxl_domain_type type = libxl__domain_type(gc, domid);
     if (type == LIBXL_DOMAIN_TYPE_INVALID) {
@@ -2723,24 +2735,8 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
     rc = libxl__device_from_disk(gc, domid, disk, &device);
     if (rc) goto out;
 
-    if (dm_ver == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
-        rc = libxl__qmp_insert_cdrom(gc, domid, disk);
-        if (rc) goto out;
-    }
-
     path = libxl__device_backend_path(gc, &device);
 
-    /* Sanity check: make sure the backend exists before writing here */
-    tmp = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/frontend", path));
-    if (!tmp)
-    {
-        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Internal error: %s does not exist",
-            libxl__sprintf(gc, "%s/frontend", path));
-        rc = ERROR_FAIL;
-        goto out;
-    }
-
-
     insert = flexarray_make(gc, 4, 1);
 
     flexarray_append_pair(insert, "type",
@@ -2753,19 +2749,100 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
     else
         flexarray_append_pair(insert, "params", "");
 
-    rc = libxl__xs_writev_atonce(gc, path,
-                        libxl__xs_kvs_of_flexarray(gc, insert, insert->count));
+    empty = flexarray_make(gc, 4, 1);
+    flexarray_append_pair(empty, "type",
+                          libxl__device_disk_string_of_backend(disk->backend));
+    flexarray_append_pair(empty, "params", "");
+
+    /* Note: CTX lock is already held at this point so lock hierarchy
+     * is maintained.
+     */
+    lock = libxl__lock_domain_userdata(gc, domid);
+    if (!lock) {
+        rc = ERROR_LOCK_FAIL;
+        goto out;
+    }
+
+    /* We need to eject the original image first. This is implemented
+     * by inserting empty media. JSON is not updated.
+     */
+    if (dm_ver == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
+        rc = libxl__qmp_insert_cdrom(gc, domid, &disk_empty);
+        if (rc) goto out;
+    }
+
+    for (;;) {
+        rc = libxl__xs_transaction_start(gc, &t);
+        if (rc) goto out;
+        /* Sanity check: make sure the backend exists before writing here */
+        tmp = libxl__xs_read(gc, t, libxl__sprintf(gc, "%s/frontend", path));
+        if (!tmp)
+        {
+            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Internal error: %s does not exist",
+                       libxl__sprintf(gc, "%s/frontend", path));
+            rc = ERROR_FAIL;
+            goto out;
+        }
+
+        rc = libxl__xs_writev(gc, t, path,
+                              libxl__xs_kvs_of_flexarray(gc, empty, empty->count));
+        if (rc) goto out;
+
+        rc = libxl__xs_transaction_commit(gc, &t);
+        if (!rc) break;
+        if (rc < 0) 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);
+
+    if (dm_ver == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
+        rc = libxl__qmp_insert_cdrom(gc, domid, disk);
+        if (rc) goto out;
+    }
+
+    for (;;) {
+        rc = libxl__xs_transaction_start(gc, &t);
+        if (rc) goto out;
+        /* Sanity check: make sure the backend exists before writing here */
+        tmp = libxl__xs_read(gc, t, libxl__sprintf(gc, "%s/frontend", path));
+        if (!tmp)
+        {
+            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Internal error: %s does not exist",
+                       libxl__sprintf(gc, "%s/frontend", path));
+            rc = ERROR_FAIL;
+            goto out;
+        }
+
+        rc = libxl__set_domain_configuration(gc, domid, &d_config);
+        if (rc) goto out;
+
+        rc = libxl__xs_writev(gc, t, path,
+                              libxl__xs_kvs_of_flexarray(gc, insert, insert->count));
+        if (rc) goto out;
+
+        rc = libxl__xs_transaction_commit(gc, &t);
+        if (!rc) break;
+        if (rc < 0) goto out;
+    }
+
     /* success, no actual async */
     libxl__ao_complete(egc, ao, 0);
 
     rc = 0;
 
 out:
+    libxl__xs_transaction_abort(gc, &t);
     for (i = 0; i < num; i++)
         libxl_device_disk_dispose(&disks[i]);
     free(disks);
+    libxl_device_disk_dispose(&disk_empty);
+    libxl_device_disk_dispose(&disk_saved);
+    libxl_domain_config_dispose(&d_config);
+
+    if (lock) libxl__unlock_domain_userdata(lock);
 
     if (rc) return AO_ABORT(rc);
     return AO_INPROGRESS;
-- 
1.7.10.4

[PATCH v4 for 4.5 4/8] libxl: refactor libxl_get_memory_target

[Wei Liu]

Introduce a helper function which can return both "target" node and
"static-max" node of a domain. Reimplement libxl_get_memory_target using
this helper. libxl__fill_dom0_memory_info is adjusted as well.

This helper will be used in later patch.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
change in v3:
don't introduce new public function
change in v4:
make the helper function return both nodes at the same time
---
 tools/libxl/libxl.c |   87 +++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 70 insertions(+), 17 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 89f4ce0..c809be7 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4256,7 +4256,8 @@ out:
     return rc;
 }
 
-static int libxl__fill_dom0_memory_info(libxl__gc *gc, uint32_t *target_memkb)
+static int libxl__fill_dom0_memory_info(libxl__gc *gc, uint32_t *target_memkb,
+                                        uint32_t *max_memkb)
 {
     int rc;
     libxl_dominfo info;
@@ -4290,6 +4291,17 @@ retry_transaction:
         }
     }
 
+    if (staticmax) {
+        *max_memkb = strtoul(staticmax, &endptr, 10);
+        if (*endptr != '\0') {
+            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
+                             "invalid memory static-max %s from %s\n",
+                             staticmax, max_path);
+            rc = ERROR_FAIL;
+            goto out;
+        }
+    }
+
     rc = libxl_domain_info(ctx, &info, 0);
     if (rc < 0)
         goto out;
@@ -4303,9 +4315,11 @@ retry_transaction:
                 (uint32_t) info.current_memkb);
         *target_memkb = (uint32_t) info.current_memkb;
     }
-    if (staticmax == NULL)
+    if (staticmax == NULL) {
         libxl__xs_write(gc, t, max_path, "%"PRIu32,
-                (uint32_t) info.max_memkb);
+                        (uint32_t) info.max_memkb);
+        *max_memkb = (uint32_t) info.max_memkb;
+    }
 
     if (freememslack == NULL) {
         free_mem_slack_kb = (uint32_t) (PAGE_TO_MEMKB(physinfo.total_pages) -
@@ -4336,12 +4350,12 @@ static int libxl__get_free_memory_slack(libxl__gc *gc, uint32_t *free_mem_slack)
     int rc;
     char *free_mem_slack_path = "/local/domain/0/memory/freemem-slack";
     char *free_mem_slack_s, *endptr;
-    uint32_t target_memkb;
+    uint32_t target_memkb, max_memkb;
 
 retry:
     free_mem_slack_s = libxl__xs_read(gc, XBT_NULL, free_mem_slack_path);
     if (!free_mem_slack_s) {
-        rc = libxl__fill_dom0_memory_info(gc, &target_memkb);
+        rc = libxl__fill_dom0_memory_info(gc, &target_memkb, &max_memkb);
         if (rc < 0)
             return rc;
         goto retry;
@@ -4364,6 +4378,7 @@ int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid,
     int rc = 1, abort_transaction = 0;
     uint32_t memorykb = 0, videoram = 0;
     uint32_t current_target_memkb = 0, new_target_memkb = 0;
+    uint32_t current_max_memkb = 0;
     char *memmax, *endptr, *videoram_s = NULL, *target = NULL;
     char *dompath = libxl__xs_get_dompath(gc, domid);
     xc_domaininfo_t info;
@@ -4379,7 +4394,8 @@ retry_transaction:
     if (!target && !domid) {
         if (!xs_transaction_end(ctx->xsh, t, 1))
             goto out_no_transaction;
-        rc = libxl__fill_dom0_memory_info(gc, &current_target_memkb);
+        rc = libxl__fill_dom0_memory_info(gc, &current_target_memkb,
+                                          &current_max_memkb);
         if (rc < 0)
             goto out_no_transaction;
         goto retry_transaction;
@@ -4494,38 +4510,75 @@ out_no_transaction:
     return rc;
 }
 
-int libxl_get_memory_target(libxl_ctx *ctx, uint32_t domid, uint32_t *out_target)
+/* out_target_memkb and out_max_memkb can be NULL */
+static int libxl__get_memory_target(libxl__gc *gc, uint32_t domid,
+                                    uint32_t *out_target_memkb,
+                                    uint32_t *out_max_memkb)
 {
-    GC_INIT(ctx);
-    int rc = 1;
-    char *target = NULL, *endptr = NULL;
+    int rc;
+    char *target = NULL, *static_max = NULL, *endptr = NULL;
     char *dompath = libxl__xs_get_dompath(gc, domid);
-    uint32_t target_memkb;
+    uint32_t target_memkb, max_memkb;
 
     target = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc,
-                "%s/memory/target", dompath));
-    if (!target && !domid) {
-        rc = libxl__fill_dom0_memory_info(gc, &target_memkb);
+                    "%s/memory/target", dompath));
+    static_max = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc,
+                    "%s/memory/static-max", dompath));
+
+    rc = ERROR_FAIL;
+    if ((!target || !static_max) && !domid) {
+        rc = libxl__fill_dom0_memory_info(gc, &target_memkb,
+                                          &max_memkb);
         if (rc < 0)
             goto out;
     } else if (!target) {
-        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
+        LIBXL__LOG_ERRNO(CTX, LIBXL__LOG_ERROR,
                 "cannot get target memory info from %s/memory/target\n",
                 dompath);
         goto out;
+    } else if (!static_max) {
+        LIBXL__LOG_ERRNO(CTX, LIBXL__LOG_ERROR,
+                "cannot get target memory info from %s/memory/static-max\n",
+                dompath);
+        goto out;
     } else {
         target_memkb = strtoul(target, &endptr, 10);
         if (*endptr != '\0') {
-            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
+            LIBXL__LOG_ERRNO(CTX, LIBXL__LOG_ERROR,
                     "invalid memory target %s from %s/memory/target\n",
                     target, dompath);
             goto out;
         }
+        max_memkb = strtoul(static_max, &endptr, 10);
+        if (*endptr != '\0') {
+            LIBXL__LOG_ERRNO(CTX, LIBXL__LOG_ERROR,
+                    "invalid memory target %s from %s/memory/static-max\n",
+                    static_max, dompath);
+            goto out;
+        }
+
     }
-    *out_target = target_memkb;
+
+    if (out_target_memkb)
+        *out_target_memkb = target_memkb;
+
+    if (out_max_memkb)
+        *out_max_memkb = max_memkb;
+
     rc = 0;
 
 out:
+    return rc;
+}
+
+int libxl_get_memory_target(libxl_ctx *ctx, uint32_t domid,
+                            uint32_t *out_target)
+{
+    GC_INIT(ctx);
+    int rc;
+
+    rc = libxl__get_memory_target(gc, domid, out_target, NULL);
+
     GC_FREE;
     return rc;
 }
-- 
1.7.10.4

[PATCH v4 for 4.5 5/8] libxl: introduce libxl_retrieve_domain_configuration

[Wei Liu]

Introduce a new public API to return domain configuration. This returned
configuration can be used to rebuild a domain.

Note that this configuration only describes the configuration necessary
to reproduce the guest visible state and does not necessarily include
specific decisions made by the toolstack regarding its current
incarnation (e.g. disk backend) unless they were specified by the
application when the domain was created.

With this approach we can preserve what user has provided in the
original configuration as well as valuable information from xenstore.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
change in v3:
correct target memory calculation

change in v4:
1. use new libxl__get_memory_target
2. don't call update functions for vtpm and nic -- there shouldn't be
   change of behaviour as JSON information is already up-to-date.
---
 tools/libxl/libxl.c |  185 +++++++++++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl.h |   16 +++++
 2 files changed, 201 insertions(+)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index c809be7..f796da8 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -6129,6 +6129,191 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, libxl_mac *src)
     for (i = 0; i < 6; i++)
         (*dst)[i] = (*src)[i];
 }
+
+int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
+                                        libxl_domain_config *d_config)
+{
+    GC_INIT(ctx);
+    int rc;
+    libxl__domain_userdata_lock *lock = NULL;
+
+    CTX_LOCK;
+
+    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) {
+        LOG(ERROR, "fail to get domain configuration for domain %d", domid);
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    /* Domain name */
+    {
+        char *domname;
+        domname = libxl_domid_to_name(ctx, domid);
+        if (!domname) {
+            LOG(ERROR, "fail to get domain name for domain %d", domid);
+            goto out;
+        }
+        free(d_config->c_info.name);
+        d_config->c_info.name = domname; /* steals allocation */
+    }
+
+    /* Domain UUID */
+    {
+        libxl_dominfo info;
+        rc = libxl_domain_info(ctx, &info, domid);
+        if (rc) {
+            LOG(ERROR, "fail to get domain info for domain %d", domid);
+            goto out;
+        }
+        libxl_uuid_copy(ctx, &d_config->c_info.uuid, &info.uuid);
+    }
+
+    /* Memory limits:
+     *
+     * Currently there are three memory limits:
+     *  1. "target" in xenstore (originally memory= in config file)
+     *  2. "static-max" in xenstore (originally maxmem= in config file)
+     *  3. "max_memkb" in hypervisor
+     *
+     * The third one is not visible and currently managed by
+     * toolstack. In order to rebuild a domain we only need to have
+     * "target" and "static-max".
+     */
+    {
+        uint32_t target_memkb = 0, max_memkb = 0;
+
+        /* "target" */
+        rc = libxl__get_memory_target(gc, domid, &target_memkb, &max_memkb);
+        if (rc) {
+            LOG(ERROR, "fail to get memory target for domain %d", domid);
+            goto out;
+        }
+        /* Target memory in xenstore is different from what user has
+         * asked for. The difference is video_memkb. See
+         * libxl_set_memory_target.
+         */
+        d_config->b_info.target_memkb = target_memkb +
+            d_config->b_info.video_memkb;
+
+        d_config->b_info.max_memkb = max_memkb;
+    }
+
+    /* Devices: disk, nic, vtpm, pcidev etc. */
+
+    /* The MERGE macro implements following logic:
+     * 0. retrieve JSON (done by now)
+     * 1. retrieve list of device from xenstore
+     * 2. use xenstore entries as primary reference and compare JSON
+     *    entries with them.
+     *    a. if a device is present in xenstore and in JSON, merge the
+     *       two views.
+     *    b. if a device is not present in xenstore but in JSON, delete
+     *       it from the result.
+     *    c. it's impossible to have an entry present in xenstore but
+     *       not in JSON, because we maintain an invariant that every
+     *       entry in xenstore must have a corresponding entry in JSON.
+     * 3. "merge" operates on "src" and "dst". "src" points to the
+     *    entry retrieved from xenstore while "dst" points to the entry
+     *    retrieve from JSON.
+     */
+#define MERGE(type, ptr, compare, merge)                                \
+    do {                                                                \
+        libxl_device_##type *p = NULL;                                  \
+        int i, j, num;                                                  \
+                                                                        \
+        p = libxl_device_##type##_list(CTX, domid, &num);               \
+        if (p == NULL) {                                                \
+            LOG(DEBUG,                                                  \
+                "no %s from xenstore for domain %d",                    \
+                #type, domid);                                          \
+        }                                                               \
+                                                                        \
+        for (i = 0; i < d_config->num_##ptr; i++) {                     \
+            libxl_device_##type *q = &d_config->ptr[i];                 \
+            for (j = 0; j < num; j++) {                                 \
+                if (compare(&p[j], q))                                  \
+                    break;                                              \
+            }                                                           \
+                                                                        \
+            if (j < num) {         /* found in xenstore */              \
+                libxl_device_##type *dst, *src;                         \
+                dst = q;                                                \
+                src = &p[j];                                            \
+                merge;                                                  \
+            } else {                /* not found in xenstore */         \
+                LOG(WARN,                                               \
+                "Device present in JSON but not in xenstore, ignored"); \
+                                                                        \
+                libxl_device_##type##_dispose(q);                       \
+                                                                        \
+                for (j = i; j < d_config->num_##ptr - 1; j++)           \
+                    memcpy(&d_config->ptr[j], &d_config->ptr[j+1],      \
+                           sizeof(libxl_device_##type));                \
+                                                                        \
+                d_config->ptr =                                         \
+                    libxl__realloc(NOGC, d_config->ptr,                 \
+                                   sizeof(libxl_device_##type) *        \
+                                   (d_config->num_##ptr - 1));          \
+                                                                        \
+                /* rewind counters */                                   \
+                d_config->num_##ptr--;                                  \
+                i--;                                                    \
+            }                                                           \
+        }                                                               \
+                                                                        \
+        for (i = 0; i < num; i++)                                       \
+            libxl_device_##type##_dispose(&p[i]);                       \
+        free(p);                                                        \
+    } while (0);
+
+    MERGE(nic, nics, COMPARE_DEVID, {});
+
+    MERGE(vtpm, vtpms, COMPARE_DEVID, {});
+
+    MERGE(pci, pcidevs, COMPARE_PCI, {});
+
+    /* Take care of removable device. We maintain invariant in the
+     * insert / remove operation so that:
+     * 1. if xenstore is "empty" while JSON is not, the result
+     *    is "empty"
+     * 2. if xenstore has a different media than JSON, use the
+     *    one in JSON
+     * 3. if xenstore and JSON have the same media, well, you
+     *    know the answer :-)
+     *
+     * Currently there is only one removable device -- CDROM.
+     * Look for libxl_cdrom_insert for reference.
+     */
+    MERGE(disk, disks, COMPARE_DISK, {
+            if (src->removable) {
+                if (!src->pdev_path || *src->pdev_path == '\0') {
+                    /* 1, no media in drive */
+                    free(dst->pdev_path);
+                    dst->pdev_path = libxl__strdup(NOGC, "");
+                    dst->format = LIBXL_DISK_FORMAT_EMPTY;
+                } else {
+                    /* 2 and 3, use JSON, no need to touch anything */
+                    ;
+                }
+            }
+        });
+
+#undef MERGE
+
+out:
+    if (lock) libxl__unlock_domain_userdata(lock);
+    CTX_UNLOCK;
+    GC_FREE;
+    return rc;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 5136d02..3bee659 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -368,6 +368,14 @@ typedef struct libxl__ctx libxl_ctx;
 #endif
 #endif
 
+/* LIBXL_HAVE_RETRIEVE_DOMAIN_CONFIGURATION
+ *
+ * If this is defined we have libxl_retrieve_domain_configuration which
+ * returns the current configuration of a domain, which can be used to
+ * rebuild a domain.
+ */
+#define LIBXL_HAVE_RETRIEVE_DOMAIN_CONFIGURATION 1
+
 /*
  * LIBXL_HAVE_BUILDINFO_VCPU_AFFINITY_ARRAYS
  *
@@ -870,6 +878,14 @@ int static inline libxl_domain_create_restore_0x040200(
 void libxl_domain_config_init(libxl_domain_config *d_config);
 void libxl_domain_config_dispose(libxl_domain_config *d_config);
 
+/*
+ * Retrieve domain configuration and filled it in d_config. The
+ * returned configuration can be used to rebuild a domain. It only
+ * works with DomU.
+ */
+int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
+                                        libxl_domain_config *d_config);
+
 int libxl_domain_suspend(libxl_ctx *ctx, uint32_t domid, int fd,
                          int flags, /* LIBXL_SUSPEND_* */
                          const libxl_asyncop_how *ao_how)
-- 
1.7.10.4

[PATCH v4 for 4.5 6/8] libxl: introduce libxl_userdata_unlink

[Wei Liu]

This will be used in later patch for xl to remove its "xl" userdata
file.

Both CTX lock and userdata lock are taken in this API. CTX lock is taken
to maintain locking hierarchy, but it also has a side effect to protect
against R-M-W by other threads. Userdata lock is used to protect against
domain destruction.

In general application should not rely on these internal locks to
protect its own userdata files. It should deploys its own lock if it
cares.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl.h     |   10 ++++++++++
 tools/libxl/libxl_dom.c |   26 ++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 3bee659..bc68cac 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -67,6 +67,13 @@
  * the same $(XEN_VERSION) (e.g. throughout a major release).
  */
 
+/* LIBXL_HAVE_USERDATA_UNLINK
+ *
+ * If it is defined, libxl has a library function called
+ * libxl_userdata_unlink.
+ */
+#define LIBXL_HAVE_USERDATA_UNLINK 1
+
 /* LIBXL_HAVE_CPUPOOL_QUALIFIER_TO_CPUPOOLID
  *
  * If this is defined, libxl has a library function called
@@ -1268,6 +1275,9 @@ int libxl_userdata_retrieve(libxl_ctx *ctx, uint32_t domid,
    * data_r and datalen_r may be 0.
    * On error return, *data_r and *datalen_r are undefined.
    */
+int libxl_userdata_unlink(libxl_ctx *ctx, uint32_t domid,
+                          const char *userdata_userid);
+
 
 int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo *physinfo);
 int libxl_set_vcpuaffinity(libxl_ctx *ctx, uint32_t domid, uint32_t vcpuid,
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 02384ac..ce0c4ac 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -2016,6 +2016,32 @@ out:
     return rc;
 }
 
+int libxl_userdata_unlink(libxl_ctx *ctx, uint32_t domid,
+                          const char *userdata_userid)
+{
+    GC_INIT(ctx);
+    int rc;
+
+    libxl__domain_userdata_lock *lock;
+    const char *filename;
+
+    CTX_LOCK;
+    lock = libxl__lock_domain_userdata(gc, domid);
+    if (!lock) {
+        rc = ERROR_LOCK_FAIL;
+        goto out;
+    }
+
+    filename = libxl__userdata_path(gc, domid, userdata_userid, "d");
+    if (unlink(filename)) rc = ERROR_FAIL;
+
+    libxl__unlock_domain_userdata(lock);
+out:
+    CTX_UNLOCK;
+    GC_FREE;
+    return rc;
+}
+
 /*
  * Local variables:
  * mode: C
-- 
1.7.10.4

[PATCH v4 for 4.5 7/8] xl: use libxl_retrieve_domain_configuration and JSON format

[Wei Liu]

Before this change, xl stores domain configuration in "xl" format, which
is in fact a verbatim copy of user supplied domain config.

Now libxl provides a new API to retrieve domain configuration, switch to
that new API, store configuration in JSON format.

Tests done so far (xl.{new,old} denotes xl with{,out} "libxl-json"
support):

1. xl.new create then xl.new save, hexdump saved file: domain config
   saved in JSON format
2. xl.new create, xl.new save then xl.old restore: failed on
   mandatory flag check
3. xl.new create, xl.new save then xl.new restore: succeeded
4. xl.old create, xl.old save then xl.new restore: succeeded
5. xl.new create then local migrate, receiving end xl.new: succeeded
6. xl.old create then local migrate, receiving end xl.new: succeeded

Note that "xl" config is still supported and handled when restarting a
domain. "xl" config file takes precedence over "libxl-json" in that
case, so that user who uses "config-update" to store new config file
won't have regression. All other scenarios (migration, domain listing
etc.) now use the new API.

Lastly, print out warning when users invoke "config-update" to
discourage them from using this command.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

---
change in v3:
improve error message and document wording
change in v4:
WARN -> WARNING
---
 docs/man/xl.pod.1         |    5 ++
 tools/libxl/xl_cmdimpl.c  |  124 ++++++++++++++++++++++++++++-----------------
 tools/libxl/xl_cmdtable.c |    4 +-
 3 files changed, 85 insertions(+), 48 deletions(-)

diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
index 9d1c2a5..f1e95db 100644
--- a/docs/man/xl.pod.1
+++ b/docs/man/xl.pod.1
@@ -187,6 +187,11 @@ immediate effect but will be applied when the guest is next
 restarted. This command is useful to ensure that runtime modifications
 made to the guest will be preserved when the guest is restarted.
 
+Since Xen 4.5 xl has improved capabilities to handle dynamic domain
+configuration changes and will preserve any changes made a runtime
+when necessary. Therefore it should not normally be necessary to use
+this command any more.
+
 I<configfile> has to be an absolute path to a file.
 
 B<OPTIONS>
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 26492fc..f8d226b 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -107,6 +107,8 @@ static const char migrate_report[]=
    *            from target to source
    */
 
+#define XL_MANDATORY_FLAG_JSON (1U << 0) /* config data is in JSON format */
+#define XL_MANDATORY_FLAG_ALL  (XL_MANDATORY_FLAG_JSON)
 struct save_file_header {
     char magic[32]; /* savefileheader_magic */
     /* All uint32_ts are in domain's byte order. */
@@ -1778,21 +1780,41 @@ skip_vfb:
 }
 
 static void reload_domain_config(uint32_t domid,
-                                 uint8_t **config_data, int *config_len)
+                                 libxl_domain_config *d_config)
 {
+    int rc;
     uint8_t *t_data;
     int ret, t_len;
+    libxl_domain_config d_config_new;
 
+    /* In case user has used "config-update" to store a new config
+     * file.
+     */
     ret = libxl_userdata_retrieve(ctx, domid, "xl", &t_data, &t_len);
-    if (ret) {
-        LOG("failed to retrieve guest configuration (rc=%d). "
-            "reusing old configuration", ret);
+    if (ret && errno != ENOENT) {
+        LOG("\"xl\" configuration found but failed to load\n");
+    }
+    if (t_len > 0) {
+        LOG("\"xl\" configuration found, using it\n");
+        libxl_domain_config_dispose(d_config);
+        parse_config_data("<updated>", (const char *)t_data,
+                          t_len, d_config);
+        free(t_data);
+        libxl_userdata_unlink(ctx, domid, "xl");
         return;
     }
 
-    free(*config_data);
-    *config_data = t_data;
-    *config_len = t_len;
+    libxl_domain_config_init(&d_config_new);
+    rc = libxl_retrieve_domain_configuration(ctx, domid, &d_config_new);
+    if (rc) {
+        LOG("failed to retrieve guest configuration (rc=%d). "
+            "reusing old configuration", rc);
+        libxl_domain_config_dispose(&d_config_new);
+    } else {
+        libxl_domain_config_dispose(d_config);
+        /* Steal allocations */
+        memcpy(d_config, &d_config_new, sizeof(libxl_domain_config));
+    }
 }
 
 /* Returns 1 if domain should be restarted,
@@ -1800,7 +1822,6 @@ static void reload_domain_config(uint32_t domid,
  * Can update r_domid if domain is destroyed etc */
 static int handle_domain_death(uint32_t *r_domid,
                                libxl_event *event,
-                               uint8_t **config_data, int *config_len,
                                libxl_domain_config *d_config)
 
 {
@@ -1858,13 +1879,12 @@ static int handle_domain_death(uint32_t *r_domid,
         break;
 
     case LIBXL_ACTION_ON_SHUTDOWN_RESTART_RENAME:
-        reload_domain_config(*r_domid, config_data, config_len);
+        reload_domain_config(*r_domid, d_config);
         restart = 2;
         break;
 
     case LIBXL_ACTION_ON_SHUTDOWN_RESTART:
-        reload_domain_config(*r_domid, config_data, config_len);
-
+        reload_domain_config(*r_domid, d_config);
         restart = 1;
         /* fall-through */
     case LIBXL_ACTION_ON_SHUTDOWN_DESTROY:
@@ -2061,6 +2081,7 @@ static uint32_t create_domain(struct domain_create *dom_info)
     const char *config_source = NULL;
     const char *restore_source = NULL;
     int migrate_fd = dom_info->migrate_fd;
+    bool config_in_json;
 
     int i;
     int need_daemon = daemonize;
@@ -2115,7 +2136,7 @@ static uint32_t create_domain(struct domain_create *dom_info)
                 restore_source, hdr.mandatory_flags, hdr.optional_flags,
                 hdr.optional_data_len);
 
-        badflags = hdr.mandatory_flags & ~( 0 /* none understood yet */ );
+        badflags = hdr.mandatory_flags & ~XL_MANDATORY_FLAG_ALL;
         if (badflags) {
             fprintf(stderr, "Savefile has mandatory flag(s) 0x%"PRIx32" "
                     "which are not supported; need newer xl\n",
@@ -2143,7 +2164,9 @@ static uint32_t create_domain(struct domain_create *dom_info)
         optdata_here = optdata_begin;
 
         if (OPTDATA_LEFT) {
-            fprintf(stderr, " Savefile contains xl domain config\n");
+            fprintf(stderr, " Savefile contains xl domain config%s\n",
+                    !!(hdr.mandatory_flags & XL_MANDATORY_FLAG_JSON)
+                    ? " in JSON format" : "");
             WITH_OPTDATA(4, {
                 memcpy(u32buf.b, optdata_here, 4);
                 config_len = u32buf.u32;
@@ -2183,6 +2206,7 @@ static uint32_t create_domain(struct domain_create *dom_info)
                 extra_config);
         }
         config_source=config_file;
+        config_in_json = false;
     } else {
         if (!config_data) {
             fprintf(stderr, "Config file not specified and"
@@ -2190,12 +2214,18 @@ static uint32_t create_domain(struct domain_create *dom_info)
             return ERROR_INVAL;
         }
         config_source = "<saved>";
+        config_in_json = !!(hdr.mandatory_flags & XL_MANDATORY_FLAG_JSON);
     }
 
     if (!dom_info->quiet)
         printf("Parsing config from %s\n", config_source);
 
-    parse_config_data(config_source, config_data, config_len, &d_config);
+    if (config_in_json) {
+        libxl_domain_config_from_json(ctx, &d_config,
+                                      (const char *)config_data);
+    } else {
+        parse_config_data(config_source, config_data, config_len, &d_config);
+    }
 
     if (migrate_fd >= 0) {
         if (d_config.c_info.name) {
@@ -2269,14 +2299,6 @@ start:
     if ( ret )
         goto error_out;
 
-    ret = libxl_userdata_store(ctx, domid, "xl",
-                                    config_data, config_len);
-    if (ret) {
-        perror("cannot save config file");
-        ret = ERROR_FAIL;
-        goto error_out;
-    }
-
     release_lock();
 
     if (!paused)
@@ -2333,9 +2355,7 @@ start:
             LOG("Domain %d has shut down, reason code %d 0x%x", domid,
                 event->u.domain_shutdown.shutdown_reason,
                 event->u.domain_shutdown.shutdown_reason);
-            switch (handle_domain_death(&domid, event,
-                                        (uint8_t **)&config_data, &config_len,
-                                        &d_config)) {
+            switch (handle_domain_death(&domid, event, &d_config)) {
             case 2:
                 if (!preserve_domain(&domid, event, &d_config)) {
                     /* If we fail then exit leaving the old domain in place. */
@@ -2372,12 +2392,6 @@ start:
                     d_config.c_info.name = strdup(common_domname);
                 }
 
-                /* Reparse the configuration in case it has changed */
-                libxl_domain_config_dispose(&d_config);
-                libxl_domain_config_init(&d_config);
-                parse_config_data(config_source, config_data, config_len,
-                                  &d_config);
-
                 /*
                  * XXX FIXME: If this sleep is not there then domain
                  * re-creation fails sometimes.
@@ -3156,9 +3170,7 @@ static void list_domains_details(const libxl_dominfo *info, int nb_domain)
 {
     libxl_domain_config d_config;
 
-    char *config_source;
-    uint8_t *data;
-    int i, len, rc;
+    int i, rc;
 
     yajl_gen hand = NULL;
     yajl_gen_status s;
@@ -3179,24 +3191,18 @@ static void list_domains_details(const libxl_dominfo *info, int nb_domain)
         s = yajl_gen_status_ok;
 
     for (i = 0; i < nb_domain; i++) {
+        libxl_domain_config_init(&d_config);
         /* no detailed info available on dom0 */
         if (info[i].domid == 0)
             continue;
-        rc = libxl_userdata_retrieve(ctx, info[i].domid, "xl", &data, &len);
+        rc = libxl_retrieve_domain_configuration(ctx, info[i].domid, &d_config);
         if (rc)
             continue;
-        if (len == 0)
-            continue;
-        CHK_SYSCALL(asprintf(&config_source, "<domid %d data>", info[i].domid));
-        libxl_domain_config_init(&d_config);
-        parse_config_data(config_source, (char *)data, len, &d_config);
         if (default_output_format == OUTPUT_FORMAT_JSON)
             s = printf_info_one_json(hand, info[i].domid, &d_config);
         else
             printf_info_sexp(info[i].domid, &d_config);
         libxl_domain_config_dispose(&d_config);
-        free(data);
-        free(config_source);
         if (s != yajl_gen_status_ok)
             goto out;
     }
@@ -3381,22 +3387,41 @@ static void save_domain_core_begin(uint32_t domid,
                                    int *config_len_r)
 {
     int rc;
+    libxl_domain_config d_config;
+    char *config_c = 0;
 
     /* configuration file in optional data: */
 
+    libxl_domain_config_init(&d_config);
+
     if (override_config_file) {
         void *config_v = 0;
         rc = libxl_read_file_contents(ctx, override_config_file,
                                       &config_v, config_len_r);
-        *config_data_r = config_v;
+        if (rc) {
+            fprintf(stderr, "unable to read overridden config file\n");
+            exit(2);
+        }
+        parse_config_data(override_config_file, config_v, *config_len_r,
+                          &d_config);
+        free(config_v);
     } else {
-        rc = libxl_userdata_retrieve(ctx, domid, "xl",
-                                     config_data_r, config_len_r);
+        rc = libxl_retrieve_domain_configuration(ctx, domid, &d_config);
+        if (rc) {
+            fprintf(stderr, "unable to retrieve domain configuration\n");
+            exit(2);
+        }
     }
-    if (rc) {
-        fputs("Unable to get config file\n",stderr);
+
+    config_c = libxl_domain_config_to_json(ctx, &d_config);
+    if (!config_c) {
+        fprintf(stderr, "unable to convert config file to JSON\n");
         exit(2);
     }
+    *config_data_r = (uint8_t *)config_c;
+    *config_len_r = strlen(config_c) + 1; /* including trailing '\0' */
+
+    libxl_domain_config_dispose(&d_config);
 }
 
 static void save_domain_core_writeconfig(int fd, const char *source,
@@ -3424,6 +3449,8 @@ static void save_domain_core_writeconfig(int fd, const char *source,
     u32buf.u32 = config_len;
     ADD_OPTDATA(u32buf.b,    4);
     ADD_OPTDATA(config_data, config_len);
+    if (config_len)
+        hdr.mandatory_flags |= XL_MANDATORY_FLAG_JSON;
 
     /* that's the optional data */
 
@@ -4433,6 +4460,9 @@ int main_config_update(int argc, char **argv)
         exit(1);
     }
 
+    fprintf(stderr, "WARNING: xl now has better capability to manage domain configuration, "
+            "avoid using this command when possible\n");
+
     domid = find_domain(argv[1]);
     argc--; argv++;
 
diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
index 7b7fa92..35f02c4 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -38,7 +38,9 @@ struct cmd_spec cmd_table[] = {
     { "config-update",
       &main_config_update, 1, 1,
       "Update a running domain's saved configuration, used when rebuilding "
-      "the domain after reboot",
+      "the domain after reboot.\n"
+      "WARNING: xl now has better capability to manage domain configuration, "
+      "avoid using this command when possible",
       "<Domain> <ConfigFile> [options] [vars]",
       "-h                      Print this help.\n"
       "-f FILE, --defconfig=FILE\n                     Use the given configuration file.\n"
-- 
1.7.10.4

[PATCH v4 for 4.5 8/8] xl: long output of "list" command now contains Dom0 information

[Wei Liu]

As we've already generated a JSON config for Dom0, print that out when
requested.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/xl_cmdimpl.c |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index f8d226b..698b3bc 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3192,9 +3192,6 @@ static void list_domains_details(const libxl_dominfo *info, int nb_domain)
 
     for (i = 0; i < nb_domain; i++) {
         libxl_domain_config_init(&d_config);
-        /* no detailed info available on dom0 */
-        if (info[i].domid == 0)
-            continue;
         rc = libxl_retrieve_domain_configuration(ctx, info[i].domid, &d_config);
         if (rc)
             continue;
-- 
1.7.10.4

Re: [PATCH v4 for 4.5 1/8] libxl: rework domain userdata file lock

[Ian Campbell]

On Tue, 2014-09-16 at 11:01 +0100, Wei Liu wrote:
> The lock introduced in d2cd9d4f ("libxl: functions to lock / unlock
> libxl userdata store") has a bug that can leak the lock file when domain
> destruction races with other functions that try to get hold of the lock.
> 
> There are several issues:
> 1. The lock is released too early with libxl__userdata_destroyall
>    deletes everything in userdata store, including the lock file.
> 2. The check of domain existence is only done at the beginning of lock
>    function, by the time the lock is acquired, the domain might have
>    been gone already.
> 
> The effect of this two issues is we can run into such situation:
> 
>      Process 1                        Process 2 domain destruction
>    # LOCK FUNCTION                 # LOCK FUNCTION
>     check domain existence          check domain existence
>                                     acquire lock (file created)
>                                    # LOCK FUNCTION
>                                     destroy all files (lock file deleted,
>                                                        lock released)
>     acquire lock (file created)
>    # LOCK FUNCTION                  destroy domain
>                                    # UNLOCK (close fd only)
>    [ lock file leaked ]
> 
> Fix this problem by deploying following changes:
> 
> 1. Unlink lock file in unlock function.
> 2. Modify libxl__userdata_destroyall to not delete domain-userdata-lock,
>    so that the lock remains held until unlock function is called.
> 3. Check domain still exists when the lock is acquired, unlock if
>    domain is already gone.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Iff you end up resending the series (I'm hoping not) it might be nice to
s/lock_carefd/carefd/ in the struct (since the usages are lock->thing so
the lock prefix is redundant). Also in the unlock perhaps comment that
the unlock must be done first before the close, since that is quite
important.

Ian

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

[Ian Campbell]

On Tue, 2014-09-16 at 11:01 +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 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>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH v4 for 4.5 3/8] libxl: make libxl_cd_insert "eject" + "insert"

[Ian Campbell]

On Tue, 2014-09-16 at 11:01 +0100, Wei Liu wrote:


Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH v4 for 4.5 4/8] libxl: refactor libxl_get_memory_target

[Ian Campbell]

On Tue, 2014-09-16 at 11:01 +0100, Wei Liu wrote:
> -static int libxl__fill_dom0_memory_info(libxl__gc *gc, uint32_t *target_memkb)
> +static int libxl__fill_dom0_memory_info(libxl__gc *gc, uint32_t *target_memkb,
> +                                        uint32_t *max_memkb)

Looking at the callers I think you could have made this tolerate NULL
too, since the callers don't all need both AFAICT. But regardless:

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH v4 for 4.5 5/8] libxl: introduce libxl_retrieve_domain_configuration

[Ian Campbell]

On Tue, 2014-09-16 at 11:01 +0100, Wei Liu wrote:
> Introduce a new public API to return domain configuration. This returned
> configuration can be used to rebuild a domain.
> 
> Note that this configuration only describes the configuration necessary
> to reproduce the guest visible state and does not necessarily include
> specific decisions made by the toolstack regarding its current
> incarnation (e.g. disk backend) unless they were specified by the
> application when the domain was created.
> 
> With this approach we can preserve what user has provided in the
> original configuration as well as valuable information from xenstore.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH v4 for 4.5 1/8] libxl: rework domain userdata file lock

[Wei Liu]

On Tue, Sep 16, 2014 at 07:50:44PM +0100, Ian Campbell wrote:
> On Tue, 2014-09-16 at 11:01 +0100, Wei Liu wrote:
> > The lock introduced in d2cd9d4f ("libxl: functions to lock / unlock
> > libxl userdata store") has a bug that can leak the lock file when domain
> > destruction races with other functions that try to get hold of the lock.
> > 
> > There are several issues:
> > 1. The lock is released too early with libxl__userdata_destroyall
> >    deletes everything in userdata store, including the lock file.
> > 2. The check of domain existence is only done at the beginning of lock
> >    function, by the time the lock is acquired, the domain might have
> >    been gone already.
> > 
> > The effect of this two issues is we can run into such situation:
> > 
> >      Process 1                        Process 2 domain destruction
> >    # LOCK FUNCTION                 # LOCK FUNCTION
> >     check domain existence          check domain existence
> >                                     acquire lock (file created)
> >                                    # LOCK FUNCTION
> >                                     destroy all files (lock file deleted,
> >                                                        lock released)
> >     acquire lock (file created)
> >    # LOCK FUNCTION                  destroy domain
> >                                    # UNLOCK (close fd only)
> >    [ lock file leaked ]
> > 
> > Fix this problem by deploying following changes:
> > 
> > 1. Unlink lock file in unlock function.
> > 2. Modify libxl__userdata_destroyall to not delete domain-userdata-lock,
> >    so that the lock remains held until unlock function is called.
> > 3. Check domain still exists when the lock is acquired, unlock if
> >    domain is already gone.
> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 

Thanks!

> Iff you end up resending the series (I'm hoping not) it might be nice to
> s/lock_carefd/carefd/ in the struct (since the usages are lock->thing so
> the lock prefix is redundant). Also in the unlock perhaps comment that
> the unlock must be done first before the close, since that is quite
> important.
> 

I'm not going to resend this series -- xen-devel is busy enough now.
I will do it either in 4.6 or when the list is less busy.

Wei.

> Ian

Re: [PATCH v4 for 4.5 0/8] libxl: synchronise domain configuration

[Ian Campbell]

On Tue, 2014-09-16 at 11:01 +0100, Wei Liu wrote:
> Version 4 of this series based on master branch.

Applied, thanks.