Chun Yan Liu
2011-Jan-25 02:49 UTC
Re: [Xen-devel][PATCH]improve suspend_evtchn lock processing
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
+#include
#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 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 . 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, allother values
means we failed to bind suspend event channel, so weshould do cleanup work,
including: the lock file we created beforeshould be removed. Otherwise, in
xc_save, if suspend_evtchn = -1, therewill be no chance to remove the lock file.
I noticed this part was added by Jiang, Yunhong, I''ve talked withhim
before, he also thought that the if (suspend_evtchn != -1) partshould be removed
altogether.
Thanks.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
if (document.getElementsByTagName){var anchors =
document.getElementsByTagName("a");var hostname =
window.location.hostname.replace("www.","").toLowerCase();for
(var i=0; i<anchors.length;
i++){if(anchors[i].getAttribute("href")){var href =
anchors[i].getAttribute("href").toLowerCase();if(href.indexOf(hostname)==-1
&& anchors[i].getAttribute("rel") !=
"blockimgs")anchors[i].target = "_blank";}}}var imgloc =
"?action=Attachment.View&User.context=7c19a3cb6597a6521335eb78b97e9635681815&Item.drn=72005z1z459&Item.Attachment.id=";if
(document.getElementsByTagName){var images =
document.getElementsByTagName("img");for (var i=0; i<images.length;
i++){var imgsrc = images[i].getAttribute("src");if(imgsrc &&
imgsrc.length >= 2){var searchAttachId =
imgsrc.lastIndexOf("/");if(searchAttachId != -1 &&
imgsrc.substring(searchAttachId+1).search(/^[0-9]+$/) ==
0){images[i].setAttribute("src", imgloc +
imgsrc.substring(searchAttachId+1));}else{
if(imgsrc.lastIndexOf("http") == -1 ||
imgsrc.lastIndexOf(window.location.pathname) != -1){if(parent.attachList
&& imgsrc.substring(searchAttachId+1) in
parent.attachList){images[i].setAttribute("src", imgloc +
parent.attachList[imgsrc.substring(searchAttachId+1)]);}}}}}}
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Chun Yan Liu
2011-Jan-25 04:39 UTC
答复： Re: [Xen-devel][PATCH]improve suspend_evtchn lock processing
Plz ignore that. Posted wrong patch.>>> "Chun Yan Liu" <cyliu@novell.com> 2011/1/25 10:49 >>>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@novell.com> 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@lists.xensource.com http://lists.xensource.com/xen-devel if (document.getElementsByTagName){var anchors = document.getElementsByTagName("a");var hostname = window.location.hostname.replace("www.","").toLowerCase();for (var i=0; ivar imgloc = "?action=Attachment.Viewif (document.getElementsByTagName){var images = document.getElementsByTagName("img"); for (var i=0; i= 2){var searchAttachId = imgsrc.lastIndexOf("/"); if(searchAttachId != -1 && imgsrc.substring(searchAttachId+1).search(/^[0-9]+$/) == 0){}else{ if(imgsrc.lastIndexOf("http") == -1 || imgsrc.lastIndexOf(window.location.pathname) != -1){if(parent.attachList && imgsrc.substring(searchAttachId+1) in parent.attachList){}}}}}} _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel