[PATCH 0/2] Two libxl misc patches

[PATCH 0/2] Two libxl misc patches

[Wei Liu]

These changes were requested by Ian Campbell when relevant patches were posted
but they didn't warrant a resend at that time.

Wei Liu (2):
  libxl_internal: lock_carefd -> carefd
  libxl_internal: comment on domain userdata unlock function

 tools/libxl/libxl_internal.c |   20 +++++++++++++++++---
 tools/libxl/libxl_internal.h |    2 +-
 2 files changed, 18 insertions(+), 4 deletions(-)

-- 
1.7.10.4

[PATCH 1/2] libxl_internal: lock_carefd -> carefd

[Wei Liu]

lock_ prefix is redundant.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/libxl_internal.c |    6 +++---
 tools/libxl/libxl_internal.h |    2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 00c3b1e..9d8025d 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -405,7 +405,7 @@ libxl__domain_userdata_lock *libxl__lock_domain_userdata(libxl__gc *gc,
         fd = open(lockfile, O_RDWR|O_CREAT, 0666);
         if (fd < 0)
             LOGE(ERROR, "cannot open lockfile %s, errno=%d", lockfile, errno);
-        lock->lock_carefd = libxl__carefd_opened(CTX, fd);
+        lock->carefd = libxl__carefd_opened(CTX, fd);
         if (fd < 0) goto out;
 
         /* Lock the file in exclusive mode, wait indefinitely to
@@ -440,7 +440,7 @@ libxl__domain_userdata_lock *libxl__lock_domain_userdata(libxl__gc *gc,
                 break;
         }
 
-        libxl__carefd_close(lock->lock_carefd);
+        libxl__carefd_close(lock->carefd);
     }
 
     /* Check the domain is still there, if not we should release the
@@ -459,7 +459,7 @@ out:
 void libxl__unlock_domain_userdata(libxl__domain_userdata_lock *lock)
 {
     if (lock->path) unlink(lock->path);
-    if (lock->lock_carefd) libxl__carefd_close(lock->lock_carefd);
+    if (lock->carefd) libxl__carefd_close(lock->carefd);
     free(lock->path);
     free(lock);
 }
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 9695f18..3a4e222 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3522,7 +3522,7 @@ int libxl__cpuid_policy_is_empty(libxl_cpuid_policy_list *pl);
 
 /* Portability note: a proper flock(2) implementation is required */
 typedef struct {
-    libxl__carefd *lock_carefd;
+    libxl__carefd *carefd;
     char *path; /* path of the lock file itself */
 } libxl__domain_userdata_lock;
 libxl__domain_userdata_lock *libxl__lock_domain_userdata(libxl__gc *gc,
-- 
1.7.10.4

Re: [PATCH 1/2] libxl_internal: lock_carefd -> carefd

[Ian Jackson]

Wei Liu writes ("[PATCH 1/2] libxl_internal: lock_carefd -> carefd"):
> lock_ prefix is redundant.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

(for 4.6)

[PATCH 2/2] libxl_internal: comment on domain userdata unlock function

[Wei Liu]

Discuss why we need to unlink file path before closes fd.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/libxl_internal.c |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 9d8025d..a70214b 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -458,6 +458,20 @@ out:
 
 void libxl__unlock_domain_userdata(libxl__domain_userdata_lock *lock)
 {
+    /* It's important to unlink the file before closing fd to avoid
+     * such race (if close before unlink):
+     *
+     *   P1 LOCK                         P2 UNLOCK
+     *   fd1 = open(lockfile)
+     *                                   close(fd2)
+     *   flock(fd1)
+     *   fstat and stat check success
+     *                                   unlink(lockfile)
+     *   return lock
+     *
+     * In above case P1 thinks it has got hold of the lock but
+     * actually lock is released by P2 (lockfile unlinked).
+     */
     if (lock->path) unlink(lock->path);
     if (lock->carefd) libxl__carefd_close(lock->carefd);
     free(lock->path);
-- 
1.7.10.4

Re: [PATCH 2/2] libxl_internal: comment on domain userdata unlock function

[Ian Jackson]

Wei Liu writes ("[PATCH 2/2] libxl_internal: comment on domain userdata unlock function"):
> Discuss why we need to unlink file path before closes fd.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
subject to minor grammar complaint:

Potential backport candidate for 4.5.1 ?

> diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> index 9d8025d..a70214b 100644
> --- a/tools/libxl/libxl_internal.c
> +++ b/tools/libxl/libxl_internal.c
> @@ -458,6 +458,20 @@ out:
>  
>  void libxl__unlock_domain_userdata(libxl__domain_userdata_lock *lock)
>  {
> +    /* It's important to unlink the file before closing fd to avoid
> +     * such race (if close before unlink):
          ^^^^

"to avoid the following race".  "Such" must refer to a thing which
precedes.

Ian.

Re: [PATCH 2/2] libxl_internal: comment on domain userdata unlock function

[Wei Liu]

On Wed, Jan 07, 2015 at 04:52:00PM +0000, Ian Jackson wrote:
> Wei Liu writes ("[PATCH 2/2] libxl_internal: comment on domain userdata unlock function"):
> > Discuss why we need to unlink file path before closes fd.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> subject to minor grammar complaint:
> 
> Potential backport candidate for 4.5.1 ?
> 

Sure, if you feel this is important.

> > diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> > index 9d8025d..a70214b 100644
> > --- a/tools/libxl/libxl_internal.c
> > +++ b/tools/libxl/libxl_internal.c
> > @@ -458,6 +458,20 @@ out:
> >  
> >  void libxl__unlock_domain_userdata(libxl__domain_userdata_lock *lock)
> >  {
> > +    /* It's important to unlink the file before closing fd to avoid
> > +     * such race (if close before unlink):
>           ^^^^
> 
> "to avoid the following race".  "Such" must refer to a thing which
> precedes.
> 

I suppose you (or Ian C) can fix this when committing? Do I need to
resend?

Wei.

> Ian.

Re: [PATCH 2/2] libxl_internal: comment on domain userdata unlock function

[Ian Campbell]

On Wed, 2015-01-07 at 16:57 +0000, Wei Liu wrote:

> > > diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> > > index 9d8025d..a70214b 100644
> > > --- a/tools/libxl/libxl_internal.c
> > > +++ b/tools/libxl/libxl_internal.c
> > > @@ -458,6 +458,20 @@ out:
> > >  
> > >  void libxl__unlock_domain_userdata(libxl__domain_userdata_lock *lock)
> > >  {
> > > +    /* It's important to unlink the file before closing fd to avoid
> > > +     * such race (if close before unlink):
> >           ^^^^
> > 
> > "to avoid the following race".  "Such" must refer to a thing which
> > precedes.
> > 
> 
> I suppose you (or Ian C) can fix this when committing? Do I need to
> resend?

I've applied both patches and fixed this up.