[Qemu-devel] [PATCH 0/4] fix qemu-nbd -c

[Qemu-devel] [PATCH 0/4] fix qemu-nbd -c

[Paolo Bonzini]

qemu-nbd -c is another casualty of removing raw_read/raw_write.  This
series fixes it.

Unfortunately, as a side effect of this qemu-nbd will have to daemonize
before detecting all possible errors.  For this reason patches 2 and 3
make qemu-nbd write errors to syslog when daemonized.

Paolo Bonzini (4):
  qemu-nbd: exit if the child exits before a socket connection is
    established
  qemu-nbd: include our own err/errx implementation
  qemu-nbd: report errors to syslog when daemonized
  qemu-nbd: do not start the block layer in the parent

 qemu-nbd.c |   78 +++++++++++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 59 insertions(+), 19 deletions(-)

-- 
1.7.6.4

[Qemu-devel] [PATCH 1/4] qemu-nbd: exit if the child exits before a socket connection is established

[Paolo Bonzini]

In -c mode qemu-nbd forks with the parent running the in-kernel NBD
client and the child running the server.  The next patch will introduce
a case in which the child fails after forking.  Detect this situation
and exit the parent.

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

diff --git a/qemu-nbd.c b/qemu-nbd.c
index d8d3e15..972524d 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -24,6 +24,7 @@
 #include <stdio.h>
 #include <getopt.h>
 #include <err.h>
+#include <sys/wait.h>
 #include <sys/types.h>
 #include <sys/socket.h>
 #include <netinet/in.h>
@@ -382,7 +383,9 @@ int main(int argc, char **argv)
             do {
                 sock = unix_socket_outgoing(socket);
                 if (sock == -1) {
-                    if (errno != ENOENT && errno != ECONNREFUSED) {
+                    /* If the child exits before we connect, fail.  */
+                    if (waitpid(pid, NULL, WNOHANG) == pid ||
+                        (errno != ENOENT && errno != ECONNREFUSED)) {
                         ret = 1;
                         goto out;
                     }
-- 
1.7.6.4

[Qemu-devel] [PATCH 2/4] qemu-nbd: include our own err/errx implementation

[Paolo Bonzini]

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

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 972524d..573bf3d 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -23,7 +23,6 @@
 #include <stdarg.h>
 #include <stdio.h>
 #include <getopt.h>
-#include <err.h>
 #include <sys/wait.h>
 #include <sys/types.h>
 #include <sys/socket.h>
@@ -78,6 +77,35 @@ static void version(const char *name)
     , name);
 }
 
+static void err(int status, const char *format, ...)
+{
+    char *s, *msg;
+    va_list ap;
+
+    msg = strerror(errno);
+    va_start(ap, format);
+    if (vasprintf(&s, format, ap) == -1) {
+	abort();
+    }
+    fprintf(stderr, "qemu-nbd: %s: %s\n", s, msg);
+    free(s);
+    exit(status);
+}
+
+static void errx(int status, const char *format, ...)
+{
+    char *s;
+    va_list ap;
+
+    va_start(ap, format);
+    if (vasprintf(&s, format, ap) == -1) {
+        abort();
+    }
+    fprintf(stderr, "qemu-nbd: %s\n", s);
+    free(s);
+    exit(status);
+}
+
 struct partition_record
 {
     uint8_t bootable;
-- 
1.7.6.4

[Qemu-devel] [PATCH 3/4] qemu-nbd: report errors to syslog when daemonized

[Paolo Bonzini]

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

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 573bf3d..5031158 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -30,6 +30,7 @@
 #include <netinet/tcp.h>
 #include <arpa/inet.h>
 #include <signal.h>
+#include <syslog.h>
 #include <libgen.h>
 
 #define SOCKET_PATH    "/var/lock/qemu-nbd-%s"
@@ -37,6 +38,7 @@
 #define NBD_BUFFER_SIZE (1024*1024)
 
 static int verbose;
+static int daemonized;
 
 static void usage(const char *name)
 {
@@ -87,7 +89,11 @@ static void err(int status, const char *format, ...)
     if (vasprintf(&s, format, ap) == -1) {
 	abort();
     }
-    fprintf(stderr, "qemu-nbd: %s: %s\n", s, msg);
+    if (daemonized) {
+        syslog(LOG_ERR, "%s: %s", s, msg);
+    } else {
+        fprintf(stderr, "qemu-nbd: %s: %s\n", s, msg);
+    }
     free(s);
     exit(status);
 }
@@ -101,7 +107,11 @@ static void errx(int status, const char *format, ...)
     if (vasprintf(&s, format, ap) == -1) {
         abort();
     }
-    fprintf(stderr, "qemu-nbd: %s\n", s);
+    if (daemonized) {
+        syslog(LOG_ERR, "%s", s);
+    } else {
+        fprintf(stderr, "qemu-nbd: %s\n", s);
+    }
     free(s);
     exit(status);
 }
@@ -387,9 +397,11 @@ int main(int argc, char **argv)
 
         if (!verbose) {
             /* detach client and server */
+            openlog("qemu-nbd", LOG_PID, LOG_USER);
             if (qemu_daemon(0, 0) == -1) {
                 err(EXIT_FAILURE, "Failed to daemonize");
             }
+            daemonized = 1;
         }
 
         if (socket == NULL) {
-- 
1.7.6.4

[Qemu-devel] [PATCH 4/4] qemu-nbd: do not start the block layer in the parent

[Paolo Bonzini]

Forking and threading do not behave very well together.  With qemu-nbd in
-c mode it could happen that the thread pool is started in the parent, and
the children (the one actually doing the I/O) is left without working I/O.
Fix this by only running bdrv_init in the child process.

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

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 5031158..962952c 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -371,21 +371,6 @@ int main(int argc, char **argv)
 	return 0;
     }
 
-    bdrv_init();
-
-    bs = bdrv_new("hda");
-
-    if ((ret = bdrv_open(bs, argv[optind], 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);
-
     if (device) {
         pid_t pid;
         int sock;
@@ -418,7 +403,6 @@ int main(int argc, char **argv)
             size_t blocksize;
 
             ret = 0;
-            bdrv_close(bs);
 
             do {
                 sock = unix_socket_outgoing(socket);
@@ -473,8 +457,21 @@ int main(int argc, char **argv)
         /* children */
     }
 
+    bdrv_init();
+    bs = bdrv_new("hda");
+    if ((ret = bdrv_open(bs, argv[optind], 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 (socket) {
         sharing_fds[0] = unix_socket_incoming(socket);
     } else {
-- 
1.7.6.4

Re: [Qemu-devel] [PATCH 4/4] qemu-nbd: do not start the block layer in the parent

[Pierre Riteau]

Thanks a lot Paolo, I confirm this patchset fixes the bug!

-- 
Pierre Riteau -- PhD student, Myriads team, IRISA, Rennes, France
http://perso.univ-rennes1.fr/pierre.riteau/

On 28 oct. 2011, at 12:17, Paolo Bonzini wrote:

> Forking and threading do not behave very well together.  With qemu-nbd in
> -c mode it could happen that the thread pool is started in the parent, and
> the children (the one actually doing the I/O) is left without working I/O.
> Fix this by only running bdrv_init in the child process.
> 
> Reported-by: Pierre Riteau <Pierre.Riteau@irisa.fr>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> qemu-nbd.c |   31 ++++++++++++++-----------------
> 1 files changed, 14 insertions(+), 17 deletions(-)
> 
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 5031158..962952c 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -371,21 +371,6 @@ int main(int argc, char **argv)
> 	return 0;
>     }
> 
> -    bdrv_init();
> -
> -    bs = bdrv_new("hda");
> -
> -    if ((ret = bdrv_open(bs, argv[optind], 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);
> -
>     if (device) {
>         pid_t pid;
>         int sock;
> @@ -418,7 +403,6 @@ int main(int argc, char **argv)
>             size_t blocksize;
> 
>             ret = 0;
> -            bdrv_close(bs);
> 
>             do {
>                 sock = unix_socket_outgoing(socket);
> @@ -473,8 +457,21 @@ int main(int argc, char **argv)
>         /* children */
>     }
> 
> +    bdrv_init();
> +    bs = bdrv_new("hda");
> +    if ((ret = bdrv_open(bs, argv[optind], 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 (socket) {
>         sharing_fds[0] = unix_socket_incoming(socket);
>     } else {
> -- 
> 1.7.6.4
> 
> 

Re: [Qemu-devel] [PATCH 4/4] qemu-nbd: do not start the block layer in the parent

[Paolo Bonzini]

On 10/28/2011 01:56 PM, Pierre Riteau wrote:
> Thanks a lot Paolo, I confirm this patchset fixes the bug!

Do you believe the limitation on error reporting to be too strong?

Paolo

Re: [Qemu-devel] [PATCH 4/4] qemu-nbd: do not start the block layer in the parent

[Pierre Riteau]

On 28 oct. 2011, at 13:57, Paolo Bonzini wrote:

> On 10/28/2011 01:56 PM, Pierre Riteau wrote:
>> Thanks a lot Paolo, I confirm this patchset fixes the bug!
> 
> Do you believe the limitation on error reporting to be too strong?
> 
> Paolo

Yes, it would be better if we could have error output on stderr. Now, "simple" errors such as a missing image file (or wrong path to the image) are reported to syslog instead. It could be the source of some headaches...

Is there a way we could have the child send the error to the parent over a pipe and have the parent print it on stderr?

Pierre

Re: [Qemu-devel] [PATCH 4/4] qemu-nbd: do not start the block layer in the parent

[Paolo Bonzini]

On 10/28/2011 02:16 PM, Pierre Riteau wrote:
> Yes, it would be better if we could have error output on stderr. Now,
> "simple" errors such as a missing image file (or wrong path to the
> image) are reported to syslog instead. It could be the source of some
> headaches...
>
> Is there a way we could have the child send the error to the parent
> over a pipe and have the parent print it on stderr?

A way could be to change the fork() into a separate thread, so that you 
can daemonize as soon as you accept the socket rather than having to do 
it early.

Paolo

Re: [Qemu-devel] [PATCH 4/4] qemu-nbd: do not start the block layer in the parent

[Paolo Bonzini]

On 10/28/2011 02:17 PM, Paolo Bonzini wrote:
>> Yes, it would be better if we could have error output on stderr. Now,
>> "simple" errors such as a missing image file (or wrong path to the
>> image) are reported to syslog instead. It could be the source of some
>> headaches...
>>
>> Is there a way we could have the child send the error to the parent
>> over a pipe and have the parent print it on stderr?
>
> A way could be to change the fork() into a separate thread, so that you
> can daemonize as soon as you accept the socket rather than having to do
> it early.

I tried implementing this, but in general daemonization (which forks and 
leave only the children) breaks the threading.

So we could either keep this series (which moves all errors to syslog, 
but doesn't otherwise change behavior), or I can finish and post an 
alternative series that removes all forking from qemu-nbd *but* changes 
behavior in that "qemu-nbd -c" will not daemonize anymore.

Since this is 1.0 after all, I'm slightly more inclined towards the latter.

Opinions?  Kevin, Anthony?

Paolo

Re: [Qemu-devel] [PATCH 4/4] qemu-nbd: do not start the block layer in the parent

[Kevin Wolf]

Am 04.11.2011 10:46, schrieb Paolo Bonzini:
> On 10/28/2011 02:17 PM, Paolo Bonzini wrote:
>>> Yes, it would be better if we could have error output on stderr. Now,
>>> "simple" errors such as a missing image file (or wrong path to the
>>> image) are reported to syslog instead. It could be the source of some
>>> headaches...
>>>
>>> Is there a way we could have the child send the error to the parent
>>> over a pipe and have the parent print it on stderr?
>>
>> A way could be to change the fork() into a separate thread, so that you
>> can daemonize as soon as you accept the socket rather than having to do
>> it early.
> 
> I tried implementing this, but in general daemonization (which forks and 
> leave only the children) breaks the threading.
> 
> So we could either keep this series (which moves all errors to syslog, 
> but doesn't otherwise change behavior), or I can finish and post an 
> alternative series that removes all forking from qemu-nbd *but* changes 
> behavior in that "qemu-nbd -c" will not daemonize anymore.
> 
> Since this is 1.0 after all, I'm slightly more inclined towards the latter.
> 
> Opinions?  Kevin, Anthony?

I'm surprised that -c is what causes trouble. As far as I understand,
the code for implementing -c doesn't use the qemu block layer at all.

So why can't we just change the code to fork before it initialises the
block layer and opens the image file?

Kevin

Re: [Qemu-devel] [PATCH 4/4] qemu-nbd: do not start the block layer in the parent

[Paolo Bonzini]

On 11/04/2011 11:31 AM, Kevin Wolf wrote:
> >  I tried implementing this, but in general daemonization (which forks and
> >  leave only the children) breaks the threading.
> >
> >  So we could either keep this series (which moves all errors to syslog,
> >  but doesn't otherwise change behavior), or I can finish and post an
> >  alternative series that removes all forking from qemu-nbd*but*  changes
> >  behavior in that "qemu-nbd -c" will not daemonize anymore.
> >
> >  Since this is 1.0 after all, I'm slightly more inclined towards the latter.
> >
> >  Opinions?  Kevin, Anthony?
>
> I'm surprised that -c is what causes trouble. As far as I understand,
> the code for implementing -c doesn't use the qemu block layer at all.

-c causes qemu-nbd to fork (both to run the server in a child process, 
and to daemonize).  The server runs in a child process, but that means 
that the server process loses the aio threads when the parent forks.

> So why can't we just change the code to fork before it initialises the
> block layer and opens the image file?

That's exactly what this series did.  However, daemonization has also to 
be done before opening the image file.  So the series has to support 
reporting errors to syslog, and also qemu-nbd will not give a nonzero 
exit status when errors occur.

Exchanging the server and client roles is also possible (i.e. put the 
server in the parent process and the client in the child).  It fixes the 
problem that the thread pool is lost in the server process.  However it 
still requires daemonization to occur before opening the image file so 
that errors cannot be reported properly.

Paolo

Re: [Qemu-devel] [PATCH 4/4] qemu-nbd: do not start the block layer in the parent

[Kevin Wolf]

Am 04.11.2011 12:10, schrieb Paolo Bonzini:
> On 11/04/2011 11:31 AM, Kevin Wolf wrote:
>>>  I tried implementing this, but in general daemonization (which forks and
>>>  leave only the children) breaks the threading.
>>>
>>>  So we could either keep this series (which moves all errors to syslog,
>>>  but doesn't otherwise change behavior), or I can finish and post an
>>>  alternative series that removes all forking from qemu-nbd*but*  changes
>>>  behavior in that "qemu-nbd -c" will not daemonize anymore.
>>>
>>>  Since this is 1.0 after all, I'm slightly more inclined towards the latter.
>>>
>>>  Opinions?  Kevin, Anthony?
>>
>> I'm surprised that -c is what causes trouble. As far as I understand,
>> the code for implementing -c doesn't use the qemu block layer at all.
> 
> -c causes qemu-nbd to fork (both to run the server in a child process, 
> and to daemonize).  The server runs in a child process, but that means 
> that the server process loses the aio threads when the parent forks.
> 
>> So why can't we just change the code to fork before it initialises the
>> block layer and opens the image file?
> 
> That's exactly what this series did.  However, daemonization has also to 
> be done before opening the image file.  So the series has to support 
> reporting errors to syslog, and also qemu-nbd will not give a nonzero 
> exit status when errors occur.

The parent could wait until the initialisation is done. As long as it
runs, writing to stderr should just work, no? Or if that doesn't work,
the child could use a pipe to communicate any errors to the parent in
the startup phase and only after the initialisation has completed it
would switch to using syslog.

> Exchanging the server and client roles is also possible (i.e. put the 
> server in the parent process and the client in the child).  It fixes the 
> problem that the thread pool is lost in the server process.  However it 
> still requires daemonization to occur before opening the image file so 
> that errors cannot be reported properly.

Yes, this doesn't solve the problem.

Kevin

Re: [Qemu-devel] [PATCH 4/4] qemu-nbd: do not start the block layer in the parent

[Paolo Bonzini]

On 11/04/2011 12:22 PM, Kevin Wolf wrote:
>> >
>> >  That's exactly what this series did.  However, daemonization has also to
>> >  be done before opening the image file.  So the series has to support
>> >  reporting errors to syslog, and also qemu-nbd will not give a nonzero
>> >  exit status when errors occur.
>
> The parent could wait until the initialisation is done.

You need to daemonize first, then fork the server.  If you fork the 
server before daemonizing, the server:

1) is not in its own process group, and still has a controlling TTY;

2) is not your child so your process structure is all broken, with the 
client and server being both child of PID 1;

3) is not your child, so you cannot reliably kill it (because if it has 
exited and PID 1 has already reaped it, you might kill a random process 
instead!).

Paolo