[Xen-devel] [PATCH v5 3/7] libxl: generalise libxl__domain_userdata_lock()

[Paul Durrant <pdurrant@amazon.com>]

This function implements a file-based lock with a file name generated
from a domid.

This patch splits it into two, generalising the core of the locking code
into a new libxl__lock_file() function which operates on a specified file,
leaving just the file name generation in libxl__domain_userdata_lock().

This patch also generalises libxl__unlock_domain_userdata() to
libxl__unlock_file() and modifies all call-sites.

Suggested-by: Ian Jackson <ian.jackson@eu.citrix.com>
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
Cc: Wei Liu <wl@xen.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>

v4:
 - New in v4.
---
 tools/libxl/libxl_create.c   |  4 +--
 tools/libxl/libxl_device.c   |  4 +--
 tools/libxl/libxl_disk.c     | 12 ++++----
 tools/libxl/libxl_dom.c      | 12 ++++----
 tools/libxl/libxl_domain.c   | 14 ++++-----
 tools/libxl/libxl_internal.c | 55 +++++++++++++++++++++---------------
 tools/libxl/libxl_internal.h | 10 ++++---
 tools/libxl/libxl_mem.c      |  8 +++---
 tools/libxl/libxl_pci.c      |  4 +--
 tools/libxl/libxl_usb.c      |  8 +++---
 10 files changed, 72 insertions(+), 59 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 1835a5502c..3a7364e2ac 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1755,7 +1755,7 @@ static void domcreate_complete(libxl__egc *egc,
     bool retain_domain = !rc || rc == ERROR_ABORTED;
 
     if (retain_domain) {
-        libxl__domain_userdata_lock *lock;
+        libxl__flock *lock;
 
         /* Note that we hold CTX lock at this point so only need to
          * take data store lock
@@ -1769,7 +1769,7 @@ static void domcreate_complete(libxl__egc *egc,
                 (gc, dcs->guest_domid, d_config_saved);
             if (!rc)
                 rc = cfg_rc;
-            libxl__unlock_domain_userdata(lock);
+            libxl__unlock_file(lock);
         }
     }
 
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 9d05d2fd13..0381c5d509 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -1850,7 +1850,7 @@ void libxl__device_add_async(libxl__egc *egc, uint32_t domid,
     xs_transaction_t t = XBT_NULL;
     libxl_domain_config d_config;
     void *type_saved;
-    libxl__domain_userdata_lock *lock = NULL;
+    libxl__flock *lock = NULL;
     int rc;
 
     libxl_domain_config_init(&d_config);
@@ -1946,7 +1946,7 @@ void libxl__device_add_async(libxl__egc *egc, uint32_t domid,
 
 out:
     libxl__xs_transaction_abort(gc, &t);
-    if (lock) libxl__unlock_domain_userdata(lock);
+    if (lock) libxl__unlock_file(lock);
     dt->dispose(type_saved);
     libxl_domain_config_dispose(&d_config);
     aodev->rc = rc;
diff --git a/tools/libxl/libxl_disk.c b/tools/libxl/libxl_disk.c
index a463334130..ddc1eec176 100644
--- a/tools/libxl/libxl_disk.c
+++ b/tools/libxl/libxl_disk.c
@@ -245,7 +245,7 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
     xs_transaction_t t = XBT_NULL;
     libxl_domain_config d_config;
     libxl_device_disk disk_saved;
-    libxl__domain_userdata_lock *lock = NULL;
+    libxl__flock *lock = NULL;
 
     libxl_domain_config_init(&d_config);
     libxl_device_disk_init(&disk_saved);
@@ -436,7 +436,7 @@ 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);
+    if (lock) libxl__unlock_file(lock);
     libxl_device_disk_dispose(&disk_saved);
     libxl_domain_config_dispose(&d_config);
     aodev->rc = rc;
@@ -794,7 +794,7 @@ static void cdrom_insert_ejected(libxl__egc *egc,
 {
     EGC_GC;
     libxl__cdrom_insert_state *cis = CONTAINER_OF(qmp, *cis, qmp);
-    libxl__domain_userdata_lock *data_lock = NULL;
+    libxl__flock *data_lock = NULL;
     libxl__device device;
     const char *be_path, *libxl_path;
     flexarray_t *empty = NULL;
@@ -896,7 +896,7 @@ static void cdrom_insert_ejected(libxl__egc *egc,
 out:
     libxl__xs_transaction_abort(gc, &t);
     libxl_domain_config_dispose(&d_config);
-    if (data_lock) libxl__unlock_domain_userdata(data_lock);
+    if (data_lock) libxl__unlock_file(data_lock);
     if (rc) {
         cdrom_insert_done(egc, cis, rc); /* must be last */
     } else if (!has_callback) {
@@ -951,7 +951,7 @@ static void cdrom_insert_inserted(libxl__egc *egc,
 {
     EGC_GC;
     libxl__cdrom_insert_state *cis = CONTAINER_OF(qmp, *cis, qmp);
-    libxl__domain_userdata_lock *data_lock = NULL;
+    libxl__flock *data_lock = NULL;
     libxl_domain_config d_config;
     flexarray_t *insert = NULL;
     xs_transaction_t t = XBT_NULL;
@@ -1029,7 +1029,7 @@ static void cdrom_insert_inserted(libxl__egc *egc,
 out:
     libxl__xs_transaction_abort(gc, &t);
     libxl_domain_config_dispose(&d_config);
-    if (data_lock) libxl__unlock_domain_userdata(data_lock);
+    if (data_lock) libxl__unlock_file(data_lock);
     cdrom_insert_done(egc, cis, rc); /* must be last */
 }
 
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 573c63692b..d9ada8a422 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1347,7 +1347,7 @@ int libxl_userdata_store(libxl_ctx *ctx, uint32_t domid,
 {
     GC_INIT(ctx);
     int rc;
-    libxl__domain_userdata_lock *lock;
+    libxl__flock *lock;
 
     CTX_LOCK;
     lock = libxl__lock_domain_userdata(gc, domid);
@@ -1359,7 +1359,7 @@ int libxl_userdata_store(libxl_ctx *ctx, uint32_t domid,
     rc = libxl__userdata_store(gc, domid, userdata_userid,
                                data, datalen);
 
-    libxl__unlock_domain_userdata(lock);
+    libxl__unlock_file(lock);
 
 out:
     CTX_UNLOCK;
@@ -1408,7 +1408,7 @@ int libxl_userdata_retrieve(libxl_ctx *ctx, uint32_t domid,
 {
     GC_INIT(ctx);
     int rc;
-    libxl__domain_userdata_lock *lock;
+    libxl__flock *lock;
 
     CTX_LOCK;
     lock = libxl__lock_domain_userdata(gc, domid);
@@ -1421,7 +1421,7 @@ int libxl_userdata_retrieve(libxl_ctx *ctx, uint32_t domid,
                                   data_r, datalen_r);
 
 
-    libxl__unlock_domain_userdata(lock);
+    libxl__unlock_file(lock);
 out:
     CTX_UNLOCK;
     GC_FREE;
@@ -1435,7 +1435,7 @@ int libxl_userdata_unlink(libxl_ctx *ctx, uint32_t domid,
     CTX_LOCK;
 
     int rc;
-    libxl__domain_userdata_lock *lock = NULL;
+    libxl__flock *lock = NULL;
     const char *filename;
 
     lock = libxl__lock_domain_userdata(gc, domid);
@@ -1458,7 +1458,7 @@ int libxl_userdata_unlink(libxl_ctx *ctx, uint32_t domid,
     rc = 0;
 out:
     if (lock)
-        libxl__unlock_domain_userdata(lock);
+        libxl__unlock_file(lock);
     CTX_UNLOCK;
     GC_FREE;
     return rc;
diff --git a/tools/libxl/libxl_domain.c b/tools/libxl/libxl_domain.c
index b59cc65750..973fc1434d 100644
--- a/tools/libxl/libxl_domain.c
+++ b/tools/libxl/libxl_domain.c
@@ -1278,7 +1278,7 @@ static void devices_destroy_cb(libxl__egc *egc,
     uint32_t domid = dis->domid;
     char *dom_path;
     char *vm_path;
-    libxl__domain_userdata_lock *lock;
+    libxl__flock *lock;
 
     dom_path = libxl__xs_get_dompath(gc, domid);
     if (!dom_path) {
@@ -1308,7 +1308,7 @@ static void devices_destroy_cb(libxl__egc *egc,
     }
     libxl__userdata_destroyall(gc, domid);
 
-    libxl__unlock_domain_userdata(lock);
+    libxl__unlock_file(lock);
 
     /* Clean up qemu-save and qemu-resume files. They are
      * intermediate files created by libxc. Unfortunately they
@@ -1917,7 +1917,7 @@ static void retrieve_domain_configuration_lock_acquired(
     retrieve_domain_configuration_state *rdcs =
         CONTAINER_OF(devlock, *rdcs, devlock);
     STATE_AO_GC(rdcs->qmp.ao);
-    libxl__domain_userdata_lock *lock = NULL;
+    libxl__flock *lock = NULL;
     bool has_callback = false;
 
     /* Convenience aliases */
@@ -1939,7 +1939,7 @@ static void retrieve_domain_configuration_lock_acquired(
         goto out;
     }
 
-    libxl__unlock_domain_userdata(lock);
+    libxl__unlock_file(lock);
     lock = NULL;
 
     /* We start by querying QEMU, if it is running, for its cpumap as this
@@ -1964,7 +1964,7 @@ static void retrieve_domain_configuration_lock_acquired(
     }
 
 out:
-    if (lock) libxl__unlock_domain_userdata(lock);
+    if (lock) libxl__unlock_file(lock);
     if (!has_callback)
         retrieve_domain_configuration_end(egc, rdcs, rc);
 }
@@ -1998,7 +1998,7 @@ static void retrieve_domain_configuration_end(libxl__egc *egc,
     retrieve_domain_configuration_state *rdcs, int rc)
 {
     STATE_AO_GC(rdcs->qmp.ao);
-    libxl__domain_userdata_lock *lock = NULL;
+    libxl__flock *lock = NULL;
 
     /* Convenience aliases */
     libxl_domain_config *const d_config = rdcs->d_config;
@@ -2205,7 +2205,7 @@ static void retrieve_domain_configuration_end(libxl__egc *egc,
 
 out:
     libxl__ev_slowlock_unlock(gc, &rdcs->devlock);
-    if (lock) libxl__unlock_domain_userdata(lock);
+    if (lock) libxl__unlock_file(lock);
     libxl_bitmap_dispose(&rdcs->qemuu_cpus);
     libxl__ev_qmp_dispose(gc, &rdcs->qmp);
     libxl__ev_time_deregister(gc, &rdcs->timeout);
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index ba5637358e..211236dc99 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -400,26 +400,22 @@ int libxl__device_model_version_running(libxl__gc *gc, uint32_t domid)
 /* Portability note: this lock utilises flock(2) so a proper implementation of
  * flock(2) is required.
  */
-libxl__domain_userdata_lock *libxl__lock_domain_userdata(libxl__gc *gc,
-                                                         uint32_t domid)
+libxl__flock *libxl__lock_file(libxl__gc *gc, const char *lockfile)
 {
-    libxl__domain_userdata_lock *lock = NULL;
-    const char *lockfile;
+    libxl__flock *lock;
     int fd;
     struct stat stab, fstab;
 
-    lockfile = libxl__userdata_path(gc, domid, "domain-userdata-lock", "l");
-    if (!lockfile) goto out;
-
-    lock = libxl__zalloc(NOGC, sizeof(libxl__domain_userdata_lock));
+    lock = libxl__zalloc(NOGC, sizeof(libxl__flock));
     lock->path = libxl__strdup(NOGC, lockfile);
 
     while (true) {
         libxl__carefd_begin();
         fd = open(lockfile, O_RDWR|O_CREAT, 0666);
         if (fd < 0)
-            LOGED(ERROR, domid,
-                  "cannot open lockfile %s, errno=%d", lockfile, errno);
+            LOGE(ERROR,
+                 "cannot open lockfile %s, errno=%d",
+                 lockfile, errno);
         lock->carefd = libxl__carefd_opened(CTX, fd);
         if (fd < 0) goto out;
 
@@ -433,21 +429,21 @@ libxl__domain_userdata_lock *libxl__lock_domain_userdata(libxl__gc *gc,
                 continue;
             default:
                 /* All other errno: EBADF, EINVAL, ENOLCK, EWOULDBLOCK */
-                LOGED(ERROR, domid,
-                      "unexpected error while trying to lock %s, fd=%d, errno=%d",
+                LOGE(ERROR,
+                     "unexpected error while trying to lock %s, fd=%d, errno=%d",
                       lockfile, fd, errno);
                 goto out;
             }
         }
 
         if (fstat(fd, &fstab)) {
-            LOGED(ERROR, domid, "cannot fstat %s, fd=%d, errno=%d",
+            LOGE(ERROR, "cannot fstat %s, fd=%d, errno=%d",
                   lockfile, fd, errno);
             goto out;
         }
         if (stat(lockfile, &stab)) {
             if (errno != ENOENT) {
-                LOGED(ERROR, domid, "cannot stat %s, errno=%d", lockfile, errno);
+                LOGE(ERROR, "cannot stat %s, errno=%d", lockfile, errno);
                 goto out;
             }
         } else {
@@ -458,20 +454,14 @@ libxl__domain_userdata_lock *libxl__lock_domain_userdata(libxl__gc *gc,
         libxl__carefd_close(lock->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 (lock) libxl__unlock_domain_userdata(lock);
+    if (lock) libxl__unlock_file(lock);
     return NULL;
 }
 
-void libxl__unlock_domain_userdata(libxl__domain_userdata_lock *lock)
+void libxl__unlock_file(libxl__flock *lock)
 {
     /* It's important to unlink the file before closing fd to avoid
      * the following race (if close before unlink):
@@ -493,6 +483,27 @@ void libxl__unlock_domain_userdata(libxl__domain_userdata_lock *lock)
     free(lock);
 }
 
+libxl__flock *libxl__lock_domain_userdata(libxl__gc *gc, uint32_t domid)
+{
+    const char *lockfile;
+    libxl__flock *lock;
+
+    lockfile = libxl__userdata_path(gc, domid, "domain-userdata-lock", "l");
+    if (!lockfile) return NULL;
+
+    lock = libxl__lock_file(gc, lockfile);
+
+    /* Check the domain is still there, if not we should release the
+     * lock and clean up.
+     */
+    if (libxl_domain_info(CTX, NULL, domid)) {
+        libxl__unlock_file(lock);
+        return NULL;
+    }
+
+    return lock;
+}
+
 int libxl__get_domain_configuration(libxl__gc *gc, uint32_t domid,
                                     libxl_domain_config *d_config)
 {
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index f2efdedfba..dd3c08bc14 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -4651,11 +4651,13 @@ int libxl__cpuid_policy_is_empty(libxl_cpuid_policy_list *pl);
 typedef struct {
     libxl__carefd *carefd;
     char *path; /* path of the lock file itself */
-} libxl__domain_userdata_lock;
+} libxl__flock;
 /* The CTX_LOCK must be held around uses of this 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);
+
+libxl__flock *libxl__lock_file(libxl__gc *gc, const char *filename);
+void libxl__unlock_file(libxl__flock *lock);
+
+libxl__flock *libxl__lock_domain_userdata(libxl__gc *gc, uint32_t domid);
 
 /*
  * Retrieve / store domain configuration from / to libxl private
diff --git a/tools/libxl/libxl_mem.c b/tools/libxl/libxl_mem.c
index 7c01fac7e5..bc7b95aa74 100644
--- a/tools/libxl/libxl_mem.c
+++ b/tools/libxl/libxl_mem.c
@@ -30,7 +30,7 @@ int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid, uint64_t max_memkb)
     uint64_t memorykb, size;
     char *dompath = libxl__xs_get_dompath(gc, domid);
     int rc = 1;
-    libxl__domain_userdata_lock *lock = NULL;
+    libxl__flock *lock = NULL;
     libxl_domain_config d_config;
 
     libxl_domain_config_init(&d_config);
@@ -85,7 +85,7 @@ int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid, uint64_t max_memkb)
     rc = 0;
 out:
     libxl_domain_config_dispose(&d_config);
-    if (lock) libxl__unlock_domain_userdata(lock);
+    if (lock) libxl__unlock_file(lock);
     CTX_UNLOCK;
     GC_FREE;
     return rc;
@@ -184,7 +184,7 @@ int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid,
     libxl_dominfo ptr;
     char *uuid;
     xs_transaction_t t;
-    libxl__domain_userdata_lock *lock;
+    libxl__flock *lock;
     libxl_domain_config d_config;
 
     libxl_domain_config_init(&d_config);
@@ -338,7 +338,7 @@ out:
 
 out_no_transaction:
     libxl_domain_config_dispose(&d_config);
-    if (lock) libxl__unlock_domain_userdata(lock);
+    if (lock) libxl__unlock_file(lock);
     CTX_UNLOCK;
     GC_FREE;
     return rc;
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 664d74c478..f91bce07ec 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -125,7 +125,7 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc,
     xs_transaction_t t = XBT_NULL;
     int rc;
     libxl_domain_config d_config;
-    libxl__domain_userdata_lock *lock = NULL;
+    libxl__flock *lock = NULL;
     bool is_stubdomain = libxl_is_stubdom(CTX, domid, NULL);
 
     /* Stubdomain doesn't have own config. */
@@ -195,7 +195,7 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc,
 
 out:
     libxl__xs_transaction_abort(gc, &t);
-    if (lock) libxl__unlock_domain_userdata(lock);
+    if (lock) libxl__unlock_file(lock);
     if (!is_stubdomain)
         libxl_domain_config_dispose(&d_config);
     return rc;
diff --git a/tools/libxl/libxl_usb.c b/tools/libxl/libxl_usb.c
index da5e3708e6..171bb04439 100644
--- a/tools/libxl/libxl_usb.c
+++ b/tools/libxl/libxl_usb.c
@@ -202,7 +202,7 @@ static int libxl__device_usbctrl_add_xenstore(libxl__gc *gc, uint32_t domid,
     int i, rc;
     libxl_domain_config d_config;
     libxl_device_usbctrl usbctrl_saved;
-    libxl__domain_userdata_lock *lock = NULL;
+    libxl__flock *lock = NULL;
 
     libxl_domain_config_init(&d_config);
     libxl_device_usbctrl_init(&usbctrl_saved);
@@ -291,7 +291,7 @@ static int libxl__device_usbctrl_add_xenstore(libxl__gc *gc, uint32_t domid,
 
 out:
     libxl__xs_transaction_abort(gc, &t);
-    if (lock) libxl__unlock_domain_userdata(lock);
+    if (lock) libxl__unlock_file(lock);
     libxl_device_usbctrl_dispose(&usbctrl_saved);
     libxl_domain_config_dispose(&d_config);
     return rc;
@@ -1266,7 +1266,7 @@ static int libxl__device_usbdev_add_xenstore(libxl__gc *gc, uint32_t domid,
     xs_transaction_t t = XBT_NULL;
     libxl_domain_config d_config;
     libxl_device_usbdev usbdev_saved;
-    libxl__domain_userdata_lock *lock = NULL;
+    libxl__flock *lock = NULL;
 
     libxl_domain_config_init(&d_config);
     libxl_device_usbdev_init(&usbdev_saved);
@@ -1323,7 +1323,7 @@ static int libxl__device_usbdev_add_xenstore(libxl__gc *gc, uint32_t domid,
     rc = 0;
 
 out:
-    if (lock) libxl__unlock_domain_userdata(lock);
+    if (lock) libxl__unlock_file(lock);
     libxl_device_usbdev_dispose(&usbdev_saved);
     libxl_domain_config_dispose(&d_config);
     return rc;
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel