[Qemu-devel] [PATCH] Improve -pidfile option

[Qemu-devel] [PATCH] Improve -pidfile option

[Anthony Liguori]

[-- Attachment #1: Type: text/plain, Size: 779 bytes --]

Howdy,

The following patch improves the -pidfile a fair bit.  -pidfile isn't 
terribly usable at the moment with -daemonize as the wrong pid gets 
written to the file.

Furthermore, there's a race in -pidfile between the stat() and initial 
create.  For a management tool, there's no 100% reliable way to know 
whether there's a stale pidfile or whether another QEMU process is running.

The attached patch uses a common technique for dealing with pidfiles.  
An advisory lock is taken once the pidfile is opened.  The nice thing 
about advisory locks is that the file is unlocked if a process dies.  
This allows for a truly atomic pidfile to be implemented.

AFAIK, lockf() exists on Windows.  I do not know if it works as one 
would expect though.

Regards,

Anthony Liguori

[-- Attachment #2: qemu-pidfile.diff --]
[-- Type: text/x-patch, Size: 2877 bytes --]

diff -r 58621c427a39 vl.c
--- a/vl.c	Sat Mar 03 21:06:59 2007 -0600
+++ b/vl.c	Sat Mar 03 21:15:21 2007 -0600
@@ -4387,44 +4387,24 @@ void usb_info(void)
     }
 }
 
-/***********************************************************/
-/* pid file */
-
-static char *pid_filename;
-
-/* Remove PID file. Called on normal exit */
-
-static void remove_pidfile(void) 
-{
-    unlink (pid_filename);
-}
-
-static void create_pidfile(const char *filename)
-{
-    struct stat pidstat;
-    FILE *f;
-
-    /* Try to write our PID to the named file */
-    if (stat(filename, &pidstat) < 0) {
-        if (errno == ENOENT) {
-            if ((f = fopen (filename, "w")) == NULL) {
-                perror("Opening pidfile");
-                exit(1);
-            }
-            fprintf(f, "%d\n", getpid());
-            fclose(f);
-            pid_filename = qemu_strdup(filename);
-            if (!pid_filename) {
-                fprintf(stderr, "Could not save PID filename");
-                exit(1);
-            }
-            atexit(remove_pidfile);
-        }
-    } else {
-        fprintf(stderr, "%s already exists. Remove it and try again.\n", 
-                filename);
-        exit(1);
-    }
+static int create_pidfile(const char *filename)
+{
+    int fd;
+    char buffer[128];
+    int len;
+
+    fd = open(filename, O_RDWR | O_CREAT, 0600);
+    if (fd == -1)
+	return -1;
+
+    if (lockf(fd, F_TLOCK, 0) == -1)
+	return -1;
+
+    len = snprintf(buffer, sizeof(buffer), "%ld\n", (long)getpid());
+    if (write(fd, buffer, len) != len)
+	return -1;
+
+    return 0;
 }
 
 /***********************************************************/
@@ -6874,6 +6854,7 @@ int main(int argc, char **argv)
     char usb_devices[MAX_USB_CMDLINE][128];
     int usb_devices_index;
     int fds[2];
+    const char *pid_file = NULL;
 
     LIST_INIT (&vm_change_state_head);
 #ifndef _WIN32
@@ -7263,7 +7244,7 @@ int main(int argc, char **argv)
                 break;
 #endif
             case QEMU_OPTION_pidfile:
-                create_pidfile(optarg);
+		pid_file = optarg;
                 break;
 #ifdef TARGET_I386
             case QEMU_OPTION_win2k_hack:
@@ -7353,9 +7334,12 @@ int main(int argc, char **argv)
 	    if (len == -1 && (errno == EINTR))
 		goto again;
 	    
-	    if (len != 1 || status != 0)
+	    if (len != 1)
 		exit(1);
-	    else
+	    else if (status == 1) {
+		fprintf(stderr, "Could not acquire pidfile\n");
+		exit(1);
+	    } else
 		exit(0);
 	} else if (pid < 0)
 	    exit(1);
@@ -7376,6 +7360,15 @@ int main(int argc, char **argv)
         signal(SIGTTIN, SIG_IGN);
     }
 #endif
+
+    if (pid_file && create_pidfile(pid_file) != 0) {
+	if (daemonize) {
+	    uint8_t status = 1;
+	    write(fds[1], &status, 1);
+	} else
+	    fprintf(stderr, "Could not acquire pid file\n");
+	exit(1);
+    }
 
 #ifdef USE_KQEMU
     if (smp_cpus > 1)