Re: [Xen-devel] [PATCH 3/9] libxl_internal: Split out userdata lock function

From: Ian Jackson <ian.jackson@citrix.com>
To: Anthony PERARD <anthony.perard@citrix.com>
Cc: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	Wei Liu <wei.liu2@citrix.com>
Subject: Re: [Xen-devel] [PATCH 3/9] libxl_internal: Split out userdata lock function
Date: Tue, 4 Jun 2019 17:55:31 +0100	[thread overview]
Message-ID: <23798.41603.907662.892014@mariner.uk.xensource.com> (raw)
In-Reply-To: <20190409164542.30274-4-anthony.perard@citrix.com>

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 <anthony.perard@citrix.com>
> ---
>  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.

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