WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel][PATCH]improve suspend_evtchn lock processing

To: <Ian.Jackson@xxxxxxxxxxxxx>
Subject: Re: [Xen-devel][PATCH]improve suspend_evtchn lock processing
From: "Chun Yan Liu" <cyliu@xxxxxxxxxx>
Date: Mon, 24 Jan 2011 19:49:50 -0700
Cc: xen-devel@xxxxxxxxxxxxxxxxxxx
Delivery-date: Mon, 24 Jan 2011 18:50:59 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
Ian, about the include header file problem, I did find it some day when I tried to make stubdom. I thought I only checked make tools before. Sorry for that.
Actually, stubdom libxc are soft links to tools/libxc/, but it uses newlibc contained in itself, not a standard gcc as in 'make tools', that newlibc does not export a flock interface.
So, I think we should not use flock, use fcntl instead.
Following is a patch using fcntl, I've tested it. Please have a check. Thanks.

diff -r 3c4c3d48a835 tools/libxc/xc_suspend.c
--- a/tools/libxc/xc_suspend.c    Thu Aug 26 11:16:56 2010 +0100
+++ b/tools/libxc/xc_suspend.c    Mon Jan 24 23:15:57 2011 +0800
@@ -16,19 +16,21 @@
 
 #include "xc_private.h"
 #include "xenguest.h"
+#include <unistd.h>
+#include <fcntl.h>
 
 #define SUSPEND_LOCK_FILE "/var/lib/xen/suspend_evtchn"
 static int lock_suspend_event(xc_interface *xch, int domid)
 {
     int fd, rc;
     mode_t mask;
-    char buf[128];
     char suspend_file[256];
+    struct flock fl;
 
     snprintf(suspend_file, sizeof(suspend_file), "%s_%d_lock.d",
         SUSPEND_LOCK_FILE, domid);
     mask = umask(022);
-    fd = open(suspend_file, O_CREAT | O_EXCL | O_RDWR, 0666);
+    fd = open(suspend_file, O_CREAT | O_RDWR, 0666);
     if (fd < 0)
     {
         ERROR("Can't create lock file for suspend event channel %s\n",
@@ -36,9 +38,11 @@
         return -EINVAL;
     }
     umask(mask);
-    snprintf(buf, sizeof(buf), "%10ld", (long)getpid());
+    memset(&fl,0,sizeof(fl));
+    fl.l_type = F_WRLCK;
+    fl.l_whence = SEEK_SET;
+    rc = fcntl(fd, F_SETLK, &fl);
 
-    rc = write_exact(fd, buf, strlen(buf));
     close(fd);
 
     return rc;
@@ -46,9 +50,9 @@
 
 static int unlock_suspend_event(xc_interface *xch, int domid)
 {
-    int fd, pid, n;
-    char buf[128];
+    int fd, rc;
     char suspend_file[256];
+    struct flock fl;
 
     snprintf(suspend_file, sizeof(suspend_file), "%s_%d_lock.d",
         SUSPEND_LOCK_FILE, domid);
@@ -57,22 +61,17 @@
     if (fd < 0)
         return -EINVAL;
 
-    n = read(fd, buf, 127);
+    memset(&fl,0,sizeof(fl));
+    fl.l_type = F_UNLCK;
+    fl.l_whence = SEEK_SET;
+    rc = fcntl(fd, F_SETLK, &fl);
 
     close(fd);
 
-    if (n > 0)
-    {
-        sscanf(buf, "%d", &pid);
-        /* We are the owner, so we can simply delete the file */
-        if (pid == getpid())
-        {
-            unlink(suspend_file);
-            return 0;
-        }
-    }
+    if(!rc)
+    unlink(suspend_file);
 
-    return -EPERM;
+    return rc;
 }
 
 int xc_await_suspend(xc_interface *xch, int xce, int suspend_evtchn)
@@ -127,8 +126,7 @@
     return suspend_evtchn;
 
 cleanup:
-    if (suspend_evtchn != -1)
-        xc_suspend_evtchn_release(xch, xce, domid, suspend_evtchn);
+    xc_suspend_evtchn_release(xch, xce, domid, suspend_evtchn);
 
     return -1;
 }



>>> Chun Yan Liu <cyliu@xxxxxxxxxx> 12/22/10 2:32 PM >>>
Sorry for delay, following is an explanation:

>> --- a/tools/libxc/xc_suspend.c    Thu Aug 26 11:16:56 2010 +0100
>> +++ b/tools/libxc/xc_suspend.c    Fri Nov 26 19:41:23 2010 +0800
>> @@ -16,19 +16,19 @@
>...
>> +#include

>That looks like a mistake.
 
I wonder if you think the header file has something wrong?
flock is defined in <sys/file.h>. To use it, need to include that header file.

>> @@ -127,8 +115,7 @@
>>      return suspend_evtchn;
>> 
>>  cleanup:
>> -    if (suspend_evtchn != -1)
>> -        xc_suspend_evtchn_release(xch, xce, domid, suspend_evtchn);
>> +    xc_suspend_evtchn_release(xch, xce, domid, suspend_evtchn);
>> 
>>      return -1;
>>  }

>What is this change for ?

if (suspend_evtchn != -1) part is unnecessary and might leave an orphan lock file.
Only suspend_evtchn > 0 means we got the lock successfully, all other values means we failed to bind suspend event channel, so we should do cleanup work, including: the lock file we created before should be removed. Otherwise, in xc_save, if suspend_evtchn = -1, there will be no chance to remove the lock file. 
I noticed this part was added by Jiang, Yunhong, I've talked with him before, he also thought that the if (suspend_evtchn != -1) part should be removed altogether.
 
Thanks.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
<Prev in Thread] Current Thread [Next in Thread>