[3/9] libxl_internal: Split out userdata lock function

Commit Message

Anthony PERARD April 9, 2019, 4:45 p.m. UTC

We are going to create a new lock and want to reuse the same machinery.
Also, hide the detail of struct libxl__domain_userdata_lock as this is
only useful as a pointer by the rest of libxl.

No functional changes.

Signed-off-by: Anthony PERARD <[e-mail]>
---
 tools/libxl/libxl_internal.c | 50 +++++++++++++++++++++++++++++-------
 tools/libxl/libxl_internal.h |  5 +---
 2 files changed, 42 insertions(+), 13 deletions(-)

Comments

Ian Jackson June 4, 2019, 4:55 p.m. UTC | #1

Anthony PERARD writes ("[PATCH 3/9] libxl_internal: Split out userdata lock function"):
> We are going to create a new lock and want to reuse the same machinery.
> Also, hide the detail of struct libxl__domain_userdata_lock as this is
> only useful as a pointer by the rest of libxl.
> 
> No functional changes.
> 
> Signed-off-by: Anthony PERARD <[e-mail]>
> ---
>  tools/libxl/libxl_internal.c | 50 +++++++++++++++++++++++++++++-------
>  tools/libxl/libxl_internal.h |  5 +---
>  2 files changed, 42 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> index f492dae5ff..fa0bbc3960 100644
> --- a/tools/libxl/libxl_internal.c
> +++ b/tools/libxl/libxl_internal.c
> @@ -397,21 +397,26 @@ int libxl__device_model_version_running(libxl__gc *gc, uint32_t domid)
>      return value;
>  }
>  
> +typedef struct {
> +    libxl__carefd *carefd;
> +    char *path; /* path of the lock file itself */
> +} libxl__generic_lock;
> +static void libxl__unlock_generic(libxl__generic_lock * const lock);
                                                          ^
Spurious space.  (Many later occurrences, too.)

> +static int libxl__lock_generic(libxl__gc *gc, libxl_domid domid,
> +                               libxl__generic_lock * const lock,
> +                               const char *lock_name)
>  {
...
> -    if (lock) libxl__unlock_domain_userdata(lock);
> -    return NULL;
> +    if (lock) libxl__unlock_generic(lock);

I think the interface to libxl__lock_generic implies !!lock, so there
is no need to test it here.

I find the legal states of a libxl__generic_lock a bit confusing.
It's only an internal struct here, but I think we need either more
forgiving rules, or explicit commentary.

As it is, the implementation of libxl__lock_generic assumes that on
entry lock->carefd and lock->path are both NULL, and there is no
function to create such a situation.  We're relying on the callers'
memsets.

> -void libxl__unlock_domain_userdata(libxl__domain_userdata_lock *lock)
> +static void libxl__unlock_generic(libxl__generic_lock * const lock)
>  {
>      /* It's important to unlink the file before closing fd to avoid
>       * the following race (if close before unlink):
> @@ -490,6 +495,33 @@ void libxl__unlock_domain_userdata(libxl__domain_userdata_lock *lock)
>      if (lock->path) unlink(lock->path);
>      if (lock->carefd) libxl__carefd_close(lock->carefd);

And this leaves lock->path and lock->carefd containing invalid
values.  Hazardous!

How about using formal state annotations ?

(I think, having looked at the one call site, that with the current
callers all follow the unwritten rules.)

Ian.

Patch

diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index f492dae5ff..fa0bbc3960 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -397,21 +397,26 @@  int libxl__device_model_version_running(libxl__gc *gc, uint32_t domid)
     return value;
 }
 
+typedef struct {
+    libxl__carefd *carefd;
+    char *path; /* path of the lock file itself */
+} libxl__generic_lock;
+static void libxl__unlock_generic(libxl__generic_lock * const lock);
+
 /* 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)
+static int libxl__lock_generic(libxl__gc *gc, libxl_domid domid,
+                               libxl__generic_lock * const lock,
+                               const char *lock_name)
 {
-    libxl__domain_userdata_lock *lock = NULL;
     const char *lockfile;
     int fd;
     struct stat stab, fstab;
 
-    lockfile = libxl__userdata_path(gc, domid, "domain-userdata-lock", "l");
+    lockfile = libxl__userdata_path(gc, domid, lock_name, "l");
     if (!lockfile) goto out;
 
-    lock = libxl__zalloc(NOGC, sizeof(libxl__domain_userdata_lock));
     lock->path = libxl__strdup(NOGC, lockfile);
 
     while (true) {
@@ -464,14 +469,14 @@  libxl__domain_userdata_lock *libxl__lock_domain_userdata(libxl__gc *gc,
     if (libxl_domain_info(CTX, NULL, domid))
         goto out;
 
-    return lock;
+    return 0;
 
 out:
-    if (lock) libxl__unlock_domain_userdata(lock);
-    return NULL;
+    if (lock) libxl__unlock_generic(lock);
+    return ERROR_FAIL;
 }
 
-void libxl__unlock_domain_userdata(libxl__domain_userdata_lock *lock)
+static void libxl__unlock_generic(libxl__generic_lock * const lock)
 {
     /* It's important to unlink the file before closing fd to avoid
      * the following race (if close before unlink):
@@ -490,6 +495,33 @@  void libxl__unlock_domain_userdata(libxl__domain_userdata_lock *lock)
     if (lock->path) unlink(lock->path);
     if (lock->carefd) libxl__carefd_close(lock->carefd);
     free(lock->path);
+}
+
+
+struct libxl__domain_userdata_lock {
+    libxl__generic_lock lock;
+};
+
+libxl__domain_userdata_lock *libxl__lock_domain_userdata(libxl__gc *gc,
+                                                         uint32_t domid)
+{
+    libxl__domain_userdata_lock *lock = NULL;
+    int rc;
+
+    lock = libxl__zalloc(NOGC, sizeof(*lock));
+    rc = libxl__lock_generic(gc, domid, &lock->lock,
+                             "domain-userdata-lock");
+    if (rc) {
+        libxl__unlock_domain_userdata(lock);
+        return NULL;
+    }
+
+    return lock;
+}
+
+void libxl__unlock_domain_userdata(libxl__domain_userdata_lock *lock)
+{
+    libxl__unlock_generic(&lock->lock);
     free(lock);
 }
 
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 702acc6d5d..f1aefaf98a 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -4486,10 +4486,7 @@  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 */
-typedef struct {
-    libxl__carefd *carefd;
-    char *path; /* path of the lock file itself */
-} libxl__domain_userdata_lock;
+typedef struct libxl__domain_userdata_lock libxl__domain_userdata_lock;
 /* 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);