qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 0/7] reorganize operation of "qemu-nbd -c" and fix it
@ 2011-11-04 14:51 Paolo Bonzini
  2011-11-04 14:51 ` [Qemu-devel] [PATCH 1/7] nbd: treat EPIPE from NBD_DO_IT as success Paolo Bonzini
                   ` (7 more replies)
  0 siblings, 8 replies; 9+ messages in thread
From: Paolo Bonzini @ 2011-11-04 14:51 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf

This is another approach to fixing the breakage of "qemu-nbd -c" due
to mixing fork with threads.  It switches operation of qemu-nbd to
threads completely, with the exception of daemonization which is moved
as early as possible to avoid conflicts with threads.

Patches 1 and 2 are bugfixes to qemu-nbd, the first more or less
unrelated, the second required by the rest of the patch.

Patches 3 and 4 change "qemu-nbd -c" to operate within a single process.
They are enough to fix the current breakage of "qemu-nbd -c -v", but not
enough for "qemu-nbd -c".  For that you need patch 5 too, which moves
the daemonization to before the block layer is initialized.

Patches 6 and 7 remove more warts that are now easily fixed.

The patches are large, but unfortunately so is the breakage.

Paolo Bonzini (7):
  nbd: treat EPIPE from NBD_DO_IT as success
  qemu-nbd: trap SIGTERM
  qemu-nbd: rename socket variable
  qemu-nbd: move client to a thread
  qemu-nbd: print error messages from the daemon through a pipe
  qemu-nbd: fix socket creation race condition
  qemu-nbd: open the block device after starting the client thread

 nbd.c      |    7 ++
 qemu-nbd.c |  289 +++++++++++++++++++++++++++++++++++++++--------------------
 2 files changed, 194 insertions(+), 102 deletions(-)

-- 
1.7.6.4

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH 1/7] nbd: treat EPIPE from NBD_DO_IT as success
  2011-11-04 14:51 [Qemu-devel] [PATCH 0/7] reorganize operation of "qemu-nbd -c" and fix it Paolo Bonzini
@ 2011-11-04 14:51 ` Paolo Bonzini
  2011-11-04 14:51 ` [Qemu-devel] [PATCH 2/7] qemu-nbd: trap SIGTERM Paolo Bonzini
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Paolo Bonzini @ 2011-11-04 14:51 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf

This can be seen with "qemu-nbd -v -c", which returns 1 instead of 0
when you disconnect with "qemu-nbd -d".

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
	I think this is a bug (because there would be no way to exit
	qemu-nbd with zero status!), but the patch can be left out safely.

 nbd.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/nbd.c b/nbd.c
index fb5e424..e6c931c 100644
--- a/nbd.c
+++ b/nbd.c
@@ -425,6 +425,13 @@ int nbd_client(int fd)
     TRACE("Doing NBD loop");
 
     ret = ioctl(fd, NBD_DO_IT);
+    if (ret == -1 && errno == EPIPE) {
+        /* NBD_DO_IT normally returns EPIPE when someone has disconnected
+         * the socket via NBD_DISCONNECT.  We do not want to return 1 in
+         * that case.
+         */
+        ret = 0;
+    }
     serrno = errno;
 
     TRACE("NBD loop returned %d: %s", ret, strerror(serrno));
-- 
1.7.6.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH 2/7] qemu-nbd: trap SIGTERM
  2011-11-04 14:51 [Qemu-devel] [PATCH 0/7] reorganize operation of "qemu-nbd -c" and fix it Paolo Bonzini
  2011-11-04 14:51 ` [Qemu-devel] [PATCH 1/7] nbd: treat EPIPE from NBD_DO_IT as success Paolo Bonzini
@ 2011-11-04 14:51 ` Paolo Bonzini
  2011-11-04 14:51 ` [Qemu-devel] [PATCH 3/7] qemu-nbd: rename socket variable Paolo Bonzini
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Paolo Bonzini @ 2011-11-04 14:51 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf

The client process right now uses SIGTERM to interrupt the server side.
This does not affect the exit status of "qemu-nbd -v -c" because the
server is a child process.  This will change when both sides will be
in the same process, and anyway cleaning up things nicely upon SIGTERM
is good practice.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-nbd.c |   34 +++++++++++++++++++++++++++++-----
 1 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index d8d3e15..d997bb0 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -36,6 +36,7 @@
 
 #define NBD_BUFFER_SIZE (1024*1024)
 
+static int sigterm_wfd;
 static int verbose;
 
 static void usage(const char *name)
@@ -163,6 +164,14 @@ static int find_partition(BlockDriverState *bs, int partition,
     return -1;
 }
 
+static void termsig_handler(int signum)
+{
+    static int sigterm_reported;
+    if (!sigterm_reported) {
+        sigterm_reported = (write(sigterm_wfd, "", 1) == 1);
+    }
+}
+
 static void show_parts(const char *device)
 {
     if (fork() == 0) {
@@ -231,6 +240,18 @@ int main(int argc, char **argv)
     int max_fd;
     int persistent = 0;
 
+    /* Set up a SIGTERM handler so that we exit with a nice status code.  */
+    struct sigaction sa_sigterm;
+    int sigterm_fd[2];
+    if (qemu_pipe(sigterm_fd) == -1) {
+        err(EXIT_FAILURE, "Error setting up communication pipe");
+    }
+
+    sigterm_wfd = sigterm_fd[1];
+    memset(&sa_sigterm, 0, sizeof(sa_sigterm));
+    sa_sigterm.sa_handler = termsig_handler;
+    sigaction(SIGTERM, &sa_sigterm, NULL);
+
     while ((ch = getopt_long(argc, argv, sopt, lopt, &opt_ind)) != -1) {
         switch (ch) {
         case 's':
@@ -423,7 +444,6 @@ int main(int argc, char **argv)
             close(fd);
  out:
             kill(pid, SIGTERM);
-            unlink(socket);
 
             return ret;
         }
@@ -444,18 +464,22 @@ int main(int argc, char **argv)
     nb_fds++;
 
     data = qemu_blockalign(bs, NBD_BUFFER_SIZE);
-    if (data == NULL)
+    if (data == NULL) {
         errx(EXIT_FAILURE, "Cannot allocate data buffer");
+    }
 
     do {
-
         FD_ZERO(&fds);
+        FD_SET(sigterm_fd[0], &fds);
         for (i = 0; i < nb_fds; i++)
             FD_SET(sharing_fds[i], &fds);
 
-        ret = select(max_fd + 1, &fds, NULL, NULL, NULL);
-        if (ret == -1)
+        do {
+            ret = select(max_fd + 1, &fds, NULL, NULL, NULL);
+        } while (ret == -1 && errno == EINTR);
+        if (ret == -1 || FD_ISSET(sigterm_fd[0], &fds)) {
             break;
+        }
 
         if (FD_ISSET(sharing_fds[0], &fds))
             ret--;
-- 
1.7.6.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH 3/7] qemu-nbd: rename socket variable
  2011-11-04 14:51 [Qemu-devel] [PATCH 0/7] reorganize operation of "qemu-nbd -c" and fix it Paolo Bonzini
  2011-11-04 14:51 ` [Qemu-devel] [PATCH 1/7] nbd: treat EPIPE from NBD_DO_IT as success Paolo Bonzini
  2011-11-04 14:51 ` [Qemu-devel] [PATCH 2/7] qemu-nbd: trap SIGTERM Paolo Bonzini
@ 2011-11-04 14:51 ` Paolo Bonzini
  2011-11-04 14:51 ` [Qemu-devel] [PATCH 4/7] qemu-nbd: move client to a thread Paolo Bonzini
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Paolo Bonzini @ 2011-11-04 14:51 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf

It will be moved to a global variable by the next patch, and it
would conflict with the socket function.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-nbd.c |   24 +++++++++++-------------
 1 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index d997bb0..ae504ec 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -202,8 +202,7 @@ int main(int argc, char **argv)
     socklen_t addr_len = sizeof(addr);
     off_t fd_size;
     char *device = NULL;
-    char *socket = NULL;
-    char sockpath[128];
+    char *sockpath = NULL;
     const char *sopt = "hVb:o:p:rsnP:c:dvk:e:t";
     struct option lopt[] = {
         { "help", 0, NULL, 'h' },
@@ -294,8 +293,8 @@ int main(int argc, char **argv)
                 errx(EXIT_FAILURE, "Invalid partition %d", partition);
             break;
         case 'k':
-            socket = optarg;
-            if (socket[0] != '/')
+            sockpath = optarg;
+            if (sockpath[0] != '/')
                 errx(EXIT_FAILURE, "socket path must be absolute\n");
             break;
         case 'd':
@@ -384,10 +383,9 @@ int main(int argc, char **argv)
             }
         }
 
-        if (socket == NULL) {
-            snprintf(sockpath, sizeof(sockpath), SOCKET_PATH,
-                     basename(device));
-            socket = sockpath;
+        if (sockpath == NULL) {
+            sockpath = g_malloc(128);
+            snprintf(sockpath, 128, SOCKET_PATH, basename(device));
         }
 
         pid = fork();
@@ -401,7 +399,7 @@ int main(int argc, char **argv)
             bdrv_close(bs);
 
             do {
-                sock = unix_socket_outgoing(socket);
+                sock = unix_socket_outgoing(sockpath);
                 if (sock == -1) {
                     if (errno != ENOENT && errno != ECONNREFUSED) {
                         ret = 1;
@@ -452,8 +450,8 @@ int main(int argc, char **argv)
 
     sharing_fds = g_malloc((shared + 1) * sizeof(int));
 
-    if (socket) {
-        sharing_fds[0] = unix_socket_incoming(socket);
+    if (sockpath) {
+        sharing_fds[0] = unix_socket_incoming(sockpath);
     } else {
         sharing_fds[0] = tcp_socket_incoming(bindto, port);
     }
@@ -515,8 +513,9 @@ int main(int argc, char **argv)
     close(sharing_fds[0]);
     bdrv_close(bs);
     g_free(sharing_fds);
-    if (socket)
-        unlink(socket);
+    if (sockpath) {
+        unlink(sockpath);
+    }
 
     return 0;
 }
-- 
1.7.6.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH 4/7] qemu-nbd: move client to a thread
  2011-11-04 14:51 [Qemu-devel] [PATCH 0/7] reorganize operation of "qemu-nbd -c" and fix it Paolo Bonzini
                   ` (2 preceding siblings ...)
  2011-11-04 14:51 ` [Qemu-devel] [PATCH 3/7] qemu-nbd: rename socket variable Paolo Bonzini
@ 2011-11-04 14:51 ` Paolo Bonzini
  2011-11-04 14:51 ` [Qemu-devel] [PATCH 5/7] qemu-nbd: print error messages from the daemon through a pipe Paolo Bonzini
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Paolo Bonzini @ 2011-11-04 14:51 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf

This avoids that qemu-nbd uses both forking and threads, which do
not behave well together.

qemu-nbd is already Unix only, and there is no qemu_thread_join,
so for now use pthreads.

Since the parent and child no longer have separate file descriptors,
we can open the NBD device before daemonizing, instead of checking
with access(2) and restricting the open to the client only.

Reported-by: Pierre Riteau <pierre.riteau@irisa.fr>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-nbd.c |  175 ++++++++++++++++++++++++++++++++----------------------------
 1 files changed, 94 insertions(+), 81 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index ae504ec..515a657 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -31,6 +31,7 @@
 #include <arpa/inet.h>
 #include <signal.h>
 #include <libgen.h>
+#include <pthread.h>
 
 #define SOCKET_PATH    "/var/lock/qemu-nbd-%s"
 
@@ -38,6 +39,9 @@
 
 static int sigterm_wfd;
 static int verbose;
+static char *device;
+static char *srcpath;
+static char *sockpath;
 
 static void usage(const char *name)
 {
@@ -172,21 +176,70 @@ static void termsig_handler(int signum)
     }
 }
 
-static void show_parts(const char *device)
+static void *show_parts(void *arg)
 {
-    if (fork() == 0) {
-        int nbd;
+    int nbd;
+
+    /* linux just needs an open() to trigger
+     * the partition table update
+     * but remember to load the module with max_part != 0 :
+     *     modprobe nbd max_part=63
+     */
+    nbd = open(device, O_RDWR);
+    if (nbd != -1) {
+        close(nbd);
+    }
+    return NULL;
+}
 
-        /* linux just needs an open() to trigger
-         * the partition table update
-         * but remember to load the module with max_part != 0 :
-         *     modprobe nbd max_part=63
-         */
-        nbd = open(device, O_RDWR);
-        if (nbd != -1)
-              close(nbd);
-        exit(0);
+static void *nbd_client_thread(void *arg)
+{
+    int fd = *(int *)arg;
+    off_t size;
+    size_t blocksize;
+    uint32_t nbdflags;
+    int sock;
+    int ret;
+    pthread_t show_parts_thread;
+
+    do {
+        sock = unix_socket_outgoing(sockpath);
+        if (sock == -1) {
+            if (errno != ENOENT && errno != ECONNREFUSED) {
+                goto out;
+            }
+            sleep(1);  /* wait parent */
+        }
+    } while (sock == -1);
+
+    ret = nbd_receive_negotiate(sock, NULL, &nbdflags,
+                                &size, &blocksize);
+    if (ret == -1) {
+        goto out;
     }
+
+    ret = nbd_init(fd, sock, nbdflags, size, blocksize);
+    if (ret == -1) {
+        goto out;
+    }
+
+    /* update partition table */
+    pthread_create(&show_parts_thread, NULL, show_parts, NULL);
+
+    fprintf(stderr, "NBD device %s is now connected to %s\n",
+            device, srcpath);
+
+    ret = nbd_client(fd);
+    if (ret) {
+        goto out;
+    }
+    close(fd);
+    kill(getpid(), SIGTERM);
+    return (void *) EXIT_SUCCESS;
+
+out:
+    kill(getpid(), SIGTERM);
+    return (void *) EXIT_FAILURE;
 }
 
 int main(int argc, char **argv)
@@ -201,8 +253,6 @@ int main(int argc, char **argv)
     struct sockaddr_in addr;
     socklen_t addr_len = sizeof(addr);
     off_t fd_size;
-    char *device = NULL;
-    char *sockpath = NULL;
     const char *sopt = "hVb:o:p:rsnP:c:dvk:e:t";
     struct option lopt[] = {
         { "help", 0, NULL, 'h' },
@@ -238,8 +288,11 @@ int main(int argc, char **argv)
     int nb_fds = 0;
     int max_fd;
     int persistent = 0;
+    pthread_t client_thread;
 
-    /* Set up a SIGTERM handler so that we exit with a nice status code.  */
+    /* The client thread uses SIGTERM to interrupt the server.  A signal
+     * handler ensures that "qemu-nbd -v -c" exits with a nice status code.
+     */
     struct sigaction sa_sigterm;
     int sigterm_fd[2];
     if (qemu_pipe(sigterm_fd) == -1) {
@@ -356,9 +409,10 @@ int main(int argc, char **argv)
 
     bs = bdrv_new("hda");
 
-    if ((ret = bdrv_open(bs, argv[optind], flags, NULL)) < 0) {
+    srcpath = argv[optind];
+    if ((ret = bdrv_open(bs, srcpath, flags, NULL)) < 0) {
         errno = -ret;
-        err(EXIT_FAILURE, "Failed to bdrv_open '%s'", argv[optind]);
+        err(EXIT_FAILURE, "Failed to bdrv_open '%s'", srcpath);
     }
 
     fd_size = bs->total_sectors * 512;
@@ -368,12 +422,14 @@ int main(int argc, char **argv)
         err(EXIT_FAILURE, "Could not find partition %d", partition);
 
     if (device) {
-        pid_t pid;
-        int sock;
+        int ret;
 
-        /* want to fail before daemonizing */
-        if (access(device, R_OK|W_OK) == -1) {
-            err(EXIT_FAILURE, "Could not access '%s'", device);
+        /* Open before spawning new threads.  In the future, we may
+         * drop privileges after opening.
+         */
+        fd = open(device, O_RDWR);
+        if (fd == -1) {
+            err(EXIT_FAILURE, "Failed to open %s", device);
         }
 
         if (!verbose) {
@@ -388,64 +444,14 @@ int main(int argc, char **argv)
             snprintf(sockpath, 128, SOCKET_PATH, basename(device));
         }
 
-        pid = fork();
-        if (pid < 0)
-            return 1;
-        if (pid != 0) {
-            off_t size;
-            size_t blocksize;
-
-            ret = 0;
-            bdrv_close(bs);
-
-            do {
-                sock = unix_socket_outgoing(sockpath);
-                if (sock == -1) {
-                    if (errno != ENOENT && errno != ECONNREFUSED) {
-                        ret = 1;
-                        goto out;
-                    }
-                    sleep(1);	/* wait children */
-                }
-            } while (sock == -1);
-
-            fd = open(device, O_RDWR);
-            if (fd == -1) {
-                ret = 1;
-                goto out;
-            }
-
-            ret = nbd_receive_negotiate(sock, NULL, &nbdflags,
-                                        &size, &blocksize);
-            if (ret == -1) {
-                ret = 1;
-                goto out;
-            }
-
-            ret = nbd_init(fd, sock, nbdflags, size, blocksize);
-            if (ret == -1) {
-                ret = 1;
-                goto out;
-            }
-
-            printf("NBD device %s is now connected to file %s\n",
-                    device, argv[optind]);
-
-	    /* update partition table */
-
-            show_parts(device);
-
-            ret = nbd_client(fd);
-            if (ret) {
-                ret = 1;
-            }
-            close(fd);
- out:
-            kill(pid, SIGTERM);
-
-            return ret;
+        ret = pthread_create(&client_thread, NULL, nbd_client_thread, &fd);
+        if (ret != 0) {
+            errx(EXIT_FAILURE, "Failed to create client thread: %s",
+                 strerror(ret));
         }
-        /* children */
+    } else {
+        /* Shut up GCC warnings.  */
+        memset(&client_thread, 0, sizeof(client_thread));
     }
 
     sharing_fds = g_malloc((shared + 1) * sizeof(int));
@@ -513,5 +519,11 @@ int main(int argc, char **argv)
         unlink(sockpath);
     }
 
-    return 0;
+    if (device) {
+        void *ret;
+        pthread_join(client_thread, &ret);
+        exit(ret != NULL);
+    } else {
+        exit(EXIT_SUCCESS);
+    }
 }
-- 
1.7.6.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH 5/7] qemu-nbd: print error messages from the daemon through a pipe
  2011-11-04 14:51 [Qemu-devel] [PATCH 0/7] reorganize operation of "qemu-nbd -c" and fix it Paolo Bonzini
                   ` (3 preceding siblings ...)
  2011-11-04 14:51 ` [Qemu-devel] [PATCH 4/7] qemu-nbd: move client to a thread Paolo Bonzini
@ 2011-11-04 14:51 ` Paolo Bonzini
  2011-11-04 14:51 ` [Qemu-devel] [PATCH 6/7] qemu-nbd: fix socket creation race Paolo Bonzini
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Paolo Bonzini @ 2011-11-04 14:51 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf

In order to get nice error messages, keep the qemu-nbd process running
until before issuing NBD_DO_IT and connected to the daemon with a pipe.
This lets the qemu-nbd process relay error messages from the daemon and
exit with a nonzero status if appropriate.

Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-nbd.c |   68 +++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 59 insertions(+), 9 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 515a657..7490008 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -225,8 +225,13 @@ static void *nbd_client_thread(void *arg)
     /* update partition table */
     pthread_create(&show_parts_thread, NULL, show_parts, NULL);
 
-    fprintf(stderr, "NBD device %s is now connected to %s\n",
-            device, srcpath);
+    if (verbose) {
+        fprintf(stderr, "NBD device %s is now connected to %s\n",
+                device, srcpath);
+    } else {
+        /* Close stderr so that the qemu-nbd process exits.  */
+        dup2(STDOUT_FILENO, STDERR_FILENO);
+    }
 
     ret = nbd_client(fd);
     if (ret) {
@@ -405,6 +410,58 @@ int main(int argc, char **argv)
 	return 0;
     }
 
+    if (device && !verbose) {
+        int stderr_fd[2];
+        pid_t pid;
+        int ret;
+
+        if (qemu_pipe(stderr_fd) == -1) {
+            err(EXIT_FAILURE, "Error setting up communication pipe");
+        }
+
+        /* Now daemonize, but keep a communication channel open to
+         * print errors and exit with the proper status code.
+         */
+        pid = fork();
+        if (pid == 0) {
+            close(stderr_fd[0]);
+            ret = qemu_daemon(0, 0);
+
+            /* Temporarily redirect stderr to the parent's pipe...  */
+            dup2(stderr_fd[1], STDERR_FILENO);
+            if (ret == -1) {
+                err(EXIT_FAILURE, "Failed to daemonize");
+            }
+
+            /* ... close the descriptor we inherited and go on.  */
+            close(stderr_fd[1]);
+        } else {
+            bool errors = false;
+            char *buf;
+
+            /* In the parent.  Print error messages from the child until
+             * it closes the pipe.
+             */
+            close(stderr_fd[1]);
+            buf = g_malloc(1024);
+            while ((ret = read(stderr_fd[0], buf, 1024)) > 0) {
+                errors = true;
+                ret = qemu_write_full(STDERR_FILENO, buf, ret);
+                if (ret == -1) {
+                    exit(EXIT_FAILURE);
+                }
+            }
+            if (ret == -1) {
+                err(EXIT_FAILURE, "Cannot read from daemon");
+            }
+
+            /* Usually the daemon should not print any message.
+             * Exit with zero status in that case.
+             */
+            exit(errors);
+        }
+    }
+
     bdrv_init();
 
     bs = bdrv_new("hda");
@@ -432,13 +494,6 @@ int main(int argc, char **argv)
             err(EXIT_FAILURE, "Failed to open %s", device);
         }
 
-        if (!verbose) {
-            /* detach client and server */
-            if (qemu_daemon(0, 0) == -1) {
-                err(EXIT_FAILURE, "Failed to daemonize");
-            }
-        }
-
         if (sockpath == NULL) {
             sockpath = g_malloc(128);
             snprintf(sockpath, 128, SOCKET_PATH, basename(device));
-- 
1.7.6.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH 6/7] qemu-nbd: fix socket creation race
  2011-11-04 14:51 [Qemu-devel] [PATCH 0/7] reorganize operation of "qemu-nbd -c" and fix it Paolo Bonzini
                   ` (4 preceding siblings ...)
  2011-11-04 14:51 ` [Qemu-devel] [PATCH 5/7] qemu-nbd: print error messages from the daemon through a pipe Paolo Bonzini
@ 2011-11-04 14:51 ` Paolo Bonzini
  2011-11-04 14:51 ` [Qemu-devel] [PATCH 7/7] qemu-nbd: open the block device after starting the client thread Paolo Bonzini
  2011-11-08 15:08 ` [Qemu-devel] [PATCH 0/7] reorganize operation of "qemu-nbd -c" and fix it Kevin Wolf
  7 siblings, 0 replies; 9+ messages in thread
From: Paolo Bonzini @ 2011-11-04 14:51 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf

Now that the client and server are in the same process, there is
no need to race on the creation of the socket.  We can open the
listening socket before starting the client thread.

This avoids that "qemu-nbd -v -c" prints this once before connecting
successfully to the socket:

    connect(unix:/var/lock/qemu-nbd-nbd0): No such file or directory

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-nbd.c |   30 +++++++++++++++---------------
 1 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 7490008..20fe4b5 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -204,10 +204,7 @@ static void *nbd_client_thread(void *arg)
     do {
         sock = unix_socket_outgoing(sockpath);
         if (sock == -1) {
-            if (errno != ENOENT && errno != ECONNREFUSED) {
-                goto out;
-            }
-            sleep(1);  /* wait parent */
+            goto out;
         }
     } while (sock == -1);
 
@@ -484,8 +481,6 @@ int main(int argc, char **argv)
         err(EXIT_FAILURE, "Could not find partition %d", partition);
 
     if (device) {
-        int ret;
-
         /* Open before spawning new threads.  In the future, we may
          * drop privileges after opening.
          */
@@ -498,15 +493,6 @@ int main(int argc, char **argv)
             sockpath = g_malloc(128);
             snprintf(sockpath, 128, SOCKET_PATH, basename(device));
         }
-
-        ret = pthread_create(&client_thread, NULL, nbd_client_thread, &fd);
-        if (ret != 0) {
-            errx(EXIT_FAILURE, "Failed to create client thread: %s",
-                 strerror(ret));
-        }
-    } else {
-        /* Shut up GCC warnings.  */
-        memset(&client_thread, 0, sizeof(client_thread));
     }
 
     sharing_fds = g_malloc((shared + 1) * sizeof(int));
@@ -519,6 +505,20 @@ int main(int argc, char **argv)
 
     if (sharing_fds[0] == -1)
         return 1;
+
+    if (device) {
+        int ret;
+
+        ret = pthread_create(&client_thread, NULL, nbd_client_thread, &fd);
+        if (ret != 0) {
+            errx(EXIT_FAILURE, "Failed to create client thread: %s",
+                 strerror(ret));
+        }
+    } else {
+        /* Shut up GCC warnings.  */
+        memset(&client_thread, 0, sizeof(client_thread));
+    }
+
     max_fd = sharing_fds[0];
     nb_fds++;
 
-- 
1.7.6.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH 7/7] qemu-nbd: open the block device after starting the client thread
  2011-11-04 14:51 [Qemu-devel] [PATCH 0/7] reorganize operation of "qemu-nbd -c" and fix it Paolo Bonzini
                   ` (5 preceding siblings ...)
  2011-11-04 14:51 ` [Qemu-devel] [PATCH 6/7] qemu-nbd: fix socket creation race Paolo Bonzini
@ 2011-11-04 14:51 ` Paolo Bonzini
  2011-11-08 15:08 ` [Qemu-devel] [PATCH 0/7] reorganize operation of "qemu-nbd -c" and fix it Kevin Wolf
  7 siblings, 0 replies; 9+ messages in thread
From: Paolo Bonzini @ 2011-11-04 14:51 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf

This is cleaner, because we do not need to close the block device when
there is an error opening /dev/nbdX.  It was done this way only to
print errors before daemonizing.

At the same time, use atexit to ensure that the block device is closed
whenever we exit.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-nbd.c |   34 +++++++++++++++++-----------------
 1 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 20fe4b5..c55b66a 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -464,22 +464,6 @@ int main(int argc, char **argv)
         }
     }
 
-    bdrv_init();
-
-    bs = bdrv_new("hda");
-
-    srcpath = argv[optind];
-    if ((ret = bdrv_open(bs, srcpath, flags, NULL)) < 0) {
-        errno = -ret;
-        err(EXIT_FAILURE, "Failed to bdrv_open '%s'", srcpath);
-    }
-
-    fd_size = bs->total_sectors * 512;
-
-    if (partition != -1 &&
-        find_partition(bs, partition, &dev_offset, &fd_size))
-        err(EXIT_FAILURE, "Could not find partition %d", partition);
-
     if (device) {
         /* Open before spawning new threads.  In the future, we may
          * drop privileges after opening.
@@ -495,6 +479,23 @@ int main(int argc, char **argv)
         }
     }
 
+    bdrv_init();
+    atexit(bdrv_close_all);
+
+    bs = bdrv_new("hda");
+    srcpath = argv[optind];
+    if ((ret = bdrv_open(bs, srcpath, flags, NULL)) < 0) {
+        errno = -ret;
+        err(EXIT_FAILURE, "Failed to bdrv_open '%s'", argv[optind]);
+    }
+
+    fd_size = bs->total_sectors * 512;
+
+    if (partition != -1 &&
+        find_partition(bs, partition, &dev_offset, &fd_size)) {
+        err(EXIT_FAILURE, "Could not find partition %d", partition);
+    }
+
     sharing_fds = g_malloc((shared + 1) * sizeof(int));
 
     if (sockpath) {
@@ -572,7 +573,6 @@ int main(int argc, char **argv)
     qemu_vfree(data);
 
     close(sharing_fds[0]);
-    bdrv_close(bs);
     g_free(sharing_fds);
     if (sockpath) {
         unlink(sockpath);
-- 
1.7.6.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH 0/7] reorganize operation of "qemu-nbd -c" and fix it
  2011-11-04 14:51 [Qemu-devel] [PATCH 0/7] reorganize operation of "qemu-nbd -c" and fix it Paolo Bonzini
                   ` (6 preceding siblings ...)
  2011-11-04 14:51 ` [Qemu-devel] [PATCH 7/7] qemu-nbd: open the block device after starting the client thread Paolo Bonzini
@ 2011-11-08 15:08 ` Kevin Wolf
  7 siblings, 0 replies; 9+ messages in thread
From: Kevin Wolf @ 2011-11-08 15:08 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel

Am 04.11.2011 15:51, schrieb Paolo Bonzini:
> This is another approach to fixing the breakage of "qemu-nbd -c" due
> to mixing fork with threads.  It switches operation of qemu-nbd to
> threads completely, with the exception of daemonization which is moved
> as early as possible to avoid conflicts with threads.
> 
> Patches 1 and 2 are bugfixes to qemu-nbd, the first more or less
> unrelated, the second required by the rest of the patch.
> 
> Patches 3 and 4 change "qemu-nbd -c" to operate within a single process.
> They are enough to fix the current breakage of "qemu-nbd -c -v", but not
> enough for "qemu-nbd -c".  For that you need patch 5 too, which moves
> the daemonization to before the block layer is initialized.
> 
> Patches 6 and 7 remove more warts that are now easily fixed.
> 
> The patches are large, but unfortunately so is the breakage.
> 
> Paolo Bonzini (7):
>   nbd: treat EPIPE from NBD_DO_IT as success
>   qemu-nbd: trap SIGTERM
>   qemu-nbd: rename socket variable
>   qemu-nbd: move client to a thread
>   qemu-nbd: print error messages from the daemon through a pipe
>   qemu-nbd: fix socket creation race condition
>   qemu-nbd: open the block device after starting the client thread
> 
>  nbd.c      |    7 ++
>  qemu-nbd.c |  289 +++++++++++++++++++++++++++++++++++++++--------------------
>  2 files changed, 194 insertions(+), 102 deletions(-)

Thanks, applied all to the block-stable branch (for 1.0)

Kevin

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2011-11-08 15:05 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-11-04 14:51 [Qemu-devel] [PATCH 0/7] reorganize operation of "qemu-nbd -c" and fix it Paolo Bonzini
2011-11-04 14:51 ` [Qemu-devel] [PATCH 1/7] nbd: treat EPIPE from NBD_DO_IT as success Paolo Bonzini
2011-11-04 14:51 ` [Qemu-devel] [PATCH 2/7] qemu-nbd: trap SIGTERM Paolo Bonzini
2011-11-04 14:51 ` [Qemu-devel] [PATCH 3/7] qemu-nbd: rename socket variable Paolo Bonzini
2011-11-04 14:51 ` [Qemu-devel] [PATCH 4/7] qemu-nbd: move client to a thread Paolo Bonzini
2011-11-04 14:51 ` [Qemu-devel] [PATCH 5/7] qemu-nbd: print error messages from the daemon through a pipe Paolo Bonzini
2011-11-04 14:51 ` [Qemu-devel] [PATCH 6/7] qemu-nbd: fix socket creation race Paolo Bonzini
2011-11-04 14:51 ` [Qemu-devel] [PATCH 7/7] qemu-nbd: open the block device after starting the client thread Paolo Bonzini
2011-11-08 15:08 ` [Qemu-devel] [PATCH 0/7] reorganize operation of "qemu-nbd -c" and fix it Kevin Wolf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).