All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH] rework daemonizing logic in qemu-nbd
@ 2012-01-14 12:39 Michael Tokarev
  2012-01-15 10:42 ` Paolo Bonzini
  0 siblings, 1 reply; 9+ messages in thread
From: Michael Tokarev @ 2012-01-14 12:39 UTC (permalink / raw)
  To: qemu-devel; +Cc: Paolo Bonzini, mjt

qemu-nbd uses daemon(3) routine to daemonize, and while
at it, it uses several hacks to make daemon(3) to work
as intended.  Instead of all these hacks, implement
daemon(3) functionality (which is a very simple function)
directly but in a way which is much more suitable for the
use case. It lets us to remove several hacks completely,
and stop using daemon() which is marked as deprecated
on e.g. MacOS.  Some more hacks around daemon(3) will
be removed in subsequent series.

Signed-Off-By: Michael Tokarev <mjt@tls.msk.ru>
---
 qemu-nbd.c |   83 ++++++++++++++++++++++++++++++++++++------------------------
 1 files changed, 50 insertions(+), 33 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index e76c782..6a84cde 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -32,6 +32,7 @@
 #include <signal.h>
 #include <libgen.h>
 #include <pthread.h>
+#include <sys/wait.h>
 
 #define SOCKET_PATH    "/var/lock/qemu-nbd-%s"
 
@@ -415,54 +416,58 @@ int main(int argc, char **argv)
     }
 
     if (device && !verbose) {
-        int stderr_fd[2];
+        int cpipe[2];
         pid_t pid;
-        int ret;
 
-        if (qemu_pipe(stderr_fd) == -1) {
+        if (qemu_pipe(cpipe) == -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.
+         * pass errors.
          */
         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) {
+            int nullfd = open("/dev/null", O_RDWR);
+            if (nullfd < 0 || setsid() < 0) {
                 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.
+            /* redirect stdin from /dev/null,
+             * stdout (temporarily) to the pipe to parent,
+             * and keep stderr for error report.
+             * When initializing is done, redirect stdout and stderr
+             * to /dev/null (stdin).
              */
-            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 (nullfd != STDIN_FILENO) {
+                dup2(nullfd, STDIN_FILENO);
+                close(nullfd);
             }
-            if (ret == -1) {
-                err(EXIT_FAILURE, "Cannot read from daemon");
+            close(cpipe[0]);
+            if (cpipe[1] != STDOUT_FILENO) {
+                dup2(cpipe[1], STDOUT_FILENO);
+                close(cpipe[1]);
             }
-
-            /* Usually the daemon should not print any message.
-             * Exit with zero status in that case.
+        } else if (pid < 0) {
+            err(EXIT_FAILURE, "Failed to daemonize");
+        } else {
+            close(cpipe[1]);
+            /* In parent, just a dummy read till the pipe gets closed.
+             * When it does, check process exit status using waitpid().
              */
-            exit(errors);
+            ret = read(cpipe[0], &ret, sizeof(ret));
+            pid = waitpid(pid, &ret, WNOHANG);
+            if (pid < 0) {
+                err(EXIT_FAILURE, "Cannot read from daemon");
+            }
+            return
+              /* waitpid(pid, WNOHANG) returns 0 if the process
+               * in question did not change state. In this case
+               * we assume our child successfully initialized and
+               * is now running, so exit succcessfully here.
+               */
+              pid == 0 ? 0 :
+              /* else our child exited, so return its exit status */
+              WIFEXITED(ret) ? WEXITSTATUS(ret) : 1;
         }
     }
 
@@ -527,6 +532,18 @@ int main(int argc, char **argv)
     qemu_set_fd_handler2(sockfd, nbd_can_accept, nbd_accept, NULL,
                          (void *)(uintptr_t)sockfd);
 
+    /* now complete the daemonizing procedure.
+     */
+    if (device && !verbose) {
+        if (chdir("/") < 0) {
+            err(EXIT_FAILURE, "unable to chdir to /");
+        }
+        /* this redirects stderr to /dev/null */
+        dup2(STDIN_FILENO, STDERR_FILENO);
+        /* this redirects stdout to /dev/null too, and closes parent pipe */
+        dup2(STDIN_FILENO, STDOUT_FILENO);
+    }
+
     do {
         main_loop_wait(false);
     } while (!sigterm_reported && (persistent || !nbd_started || nb_fds > 0));
-- 
1.7.2.5

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

* Re: [Qemu-devel] [PATCH] rework daemonizing logic in qemu-nbd
  2012-01-14 12:39 [Qemu-devel] [PATCH] rework daemonizing logic in qemu-nbd Michael Tokarev
@ 2012-01-15 10:42 ` Paolo Bonzini
  2012-01-15 12:50   ` Michael Tokarev
  0 siblings, 1 reply; 9+ messages in thread
From: Paolo Bonzini @ 2012-01-15 10:42 UTC (permalink / raw)
  To: qemu-devel

On 01/14/2012 01:39 PM, Michael Tokarev wrote:
>           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) {
> +            int nullfd = open("/dev/null", O_RDWR);
> +            if (nullfd<  0 || setsid()<  0) {
>                   err(EXIT_FAILURE, "Failed to daemonize");
>               }

This is forking only once.

> -
> -            /* ... 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.
> +            /* redirect stdin from /dev/null,
> +             * stdout (temporarily) to the pipe to parent,

This is a bit of a hack.

> +    /* now complete the daemonizing procedure.
> +     */
> +    if (device && !verbose) {
> +        if (chdir("/") < 0) {
> +            err(EXIT_FAILURE, "unable to chdir to /");
> +        }
> +        /* this redirects stderr to /dev/null */
> +        dup2(STDIN_FILENO, STDERR_FILENO);
> +        /* this redirects stdout to /dev/null too, and closes parent pipe */
> +        dup2(STDIN_FILENO, STDOUT_FILENO);
> +    }
> +

Half of this is already done in client_thread, and that would be the 
place where you should add dup2(0, 1).  Also, the chdir can be moved 
earlier, after bdrv_open.

Paolo

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

* Re: [Qemu-devel] [PATCH] rework daemonizing logic in qemu-nbd
  2012-01-15 10:42 ` Paolo Bonzini
@ 2012-01-15 12:50   ` Michael Tokarev
  2012-01-15 16:11     ` Paolo Bonzini
  0 siblings, 1 reply; 9+ messages in thread
From: Michael Tokarev @ 2012-01-15 12:50 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel

On 15.01.2012 14:42, Paolo Bonzini wrote:
> On 01/14/2012 01:39 PM, Michael Tokarev wrote:
>>           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) {
>> +            int nullfd = open("/dev/null", O_RDWR);
>> +            if (nullfd<  0 || setsid()<  0) {
>>                   err(EXIT_FAILURE, "Failed to daemonize");
>>               }
> 
> This is forking only once.

Is it good or bad?  There's no need to fork twice.  Second
fork (to the one which is already done in daemon(3)) has
been done to work around lack of proper communication between
parent and child in case of using plain daemon(3).  I.e., due
to daemon(3) interface being unflexible/unsuitable for the
current use case.

>> -
>> -            /* ... 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.
>> +            /* redirect stdin from /dev/null,
>> +             * stdout (temporarily) to the pipe to parent,
> 
> This is a bit of a hack.

There's another way -- to keep the writing pipe end in some
local variable and use that one instead of STDOUT_FILENO.
I can do it that way for sure, just thought it's already
using too much local variables.

>> +    /* now complete the daemonizing procedure.
>> +     */
>> +    if (device && !verbose) {
>> +        if (chdir("/") < 0) {
>> +            err(EXIT_FAILURE, "unable to chdir to /");
>> +        }
>> +        /* this redirects stderr to /dev/null */
>> +        dup2(STDIN_FILENO, STDERR_FILENO);
>> +        /* this redirects stdout to /dev/null too, and closes parent pipe */
>> +        dup2(STDIN_FILENO, STDOUT_FILENO);
>> +    }
>> +
> 
> Half of this is already done in client_thread, and that would be the place where you should add dup2(0, 1).

I partly disagree.

I wanted to de-couple -c (device) case with daemonizing.
client_thread only works in -c case, but daemonizing in
that case is wrong as I already pointed out in another
email - we should either stop daemonizing here at all
or have a separate option for it.

>  Also, the chdir can be moved earlier, after bdrv_open.

There's no need to, afiacs.  We complete init process and
enter main loop.  Chdir should be done befor entering main
loop, the rest makes no difference (as long as the files
we open will be accessible from cwd).

Thanks,

/mjt

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

* Re: [Qemu-devel] [PATCH] rework daemonizing logic in qemu-nbd
  2012-01-15 12:50   ` Michael Tokarev
@ 2012-01-15 16:11     ` Paolo Bonzini
  2012-01-15 16:44       ` Michael Tokarev
  0 siblings, 1 reply; 9+ messages in thread
From: Paolo Bonzini @ 2012-01-15 16:11 UTC (permalink / raw)
  To: Michael Tokarev; +Cc: qemu-devel

On 01/15/2012 01:50 PM, Michael Tokarev wrote:
> On 15.01.2012 14:42, Paolo Bonzini wrote:
>> On 01/14/2012 01:39 PM, Michael Tokarev wrote:
>>>            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) {
>>> +            int nullfd = open("/dev/null", O_RDWR);
>>> +            if (nullfd<   0 || setsid()<   0) {
>>>                    err(EXIT_FAILURE, "Failed to daemonize");
>>>                }
>>
>> This is forking only once.
>
> Is it good or bad?  There's no need to fork twice.  Second
> fork (to the one which is already done in daemon(3)) has
> been done to work around lack of proper communication between
> parent and child in case of using plain daemon(3).  I.e., due
> to daemon(3) interface being unflexible/unsuitable for the
> current use case.

daemon(3) forks twice (so qemu-nbd is effectively forking three times, 
one of which is unnecessary).

See 
http://stackoverflow.com/questions/881388/what-is-the-reason-for-performing-a-double-fork-when-creating-a-daemon 
for why there is a fork before setsid and one after.

>>> -
>>> -            /* ... 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.
>>> +            /* redirect stdin from /dev/null,
>>> +             * stdout (temporarily) to the pipe to parent,
>>
>> This is a bit of a hack.
>
> There's another way -- to keep the writing pipe end in some
> local variable and use that one instead of STDOUT_FILENO.
> I can do it that way for sure, just thought it's already
> using too much local variables.

Yes, that would be better.

>>> +    /* now complete the daemonizing procedure.
>>> +     */
>>> +    if (device&&  !verbose) {
>>> +        if (chdir("/")<  0) {
>>> +            err(EXIT_FAILURE, "unable to chdir to /");
>>> +        }
>>> +        /* this redirects stderr to /dev/null */
>>> +        dup2(STDIN_FILENO, STDERR_FILENO);
>>> +        /* this redirects stdout to /dev/null too, and closes parent pipe */
>>> +        dup2(STDIN_FILENO, STDOUT_FILENO);
>>> +    }
>>> +
>>
>> Half of this is already done in client_thread, and that would be
>> theplace where you should add dup2(0, 1).
>
> I partly disagree.
>
> I wanted to de-couple -c (device) case with daemonizing.
> client_thread only works in -c case, but daemonizing in
> that case is wrong as I already pointed out in another
> email - we should either stop daemonizing here at all
> or have a separate option for it.

We can only clean up standard file descriptors after all initialization 
tasks have been done.  nbd_client_thread could still write error 
messages.  Your patch introduces a race.

>>   Also, the chdir can be moved earlier, after bdrv_open.
>
> There's no need to, afiacs.  We complete init process and
> enter main loop.  Chdir should be done befor entering main
> loop, the rest makes no difference (as long as the files
> we open will be accessible from cwd).

Yes, but I prefer to have the chdir done unconditionally as soon as 
possible.

Paolo

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

* Re: [Qemu-devel] [PATCH] rework daemonizing logic in qemu-nbd
  2012-01-15 16:11     ` Paolo Bonzini
@ 2012-01-15 16:44       ` Michael Tokarev
  2012-01-15 17:31         ` Paolo Bonzini
  0 siblings, 1 reply; 9+ messages in thread
From: Michael Tokarev @ 2012-01-15 16:44 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel

On 15.01.2012 20:11, Paolo Bonzini wrote:
> On 01/15/2012 01:50 PM, Michael Tokarev wrote:
>> On 15.01.2012 14:42, Paolo Bonzini wrote:
>>> On 01/14/2012 01:39 PM, Michael Tokarev wrote:
>>>>            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) {
>>>> +            int nullfd = open("/dev/null", O_RDWR);
>>>> +            if (nullfd<   0 || setsid()<   0) {
>>>>                    err(EXIT_FAILURE, "Failed to daemonize");
>>>>                }
>>>
>>> This is forking only once.
>>
>> Is it good or bad?  There's no need to fork twice.  Second
>> fork (to the one which is already done in daemon(3)) has
>> been done to work around lack of proper communication between
>> parent and child in case of using plain daemon(3).  I.e., due
>> to daemon(3) interface being unflexible/unsuitable for the
>> current use case.
> 
> daemon(3) forks twice (so qemu-nbd is effectively forking three times, one of which is unnecessary).
> 
> See http://stackoverflow.com/questions/881388/what-is-the-reason-for-performing-a-double-fork-when-creating-a-daemon for why there is a fork before setsid and one after.

Daemon(3) on linux (glibc) does not try to fork twice, just
one time is sufficient.  Yes in old times there was some
portability issues on some unixes with controling terminal
and what not.  That thread summaries it up almost nicely at
the end: "So I suppose it all just boils down to tradition
in the end - a single fork is sufficient as long as the
parent dies in short order anyway," and "...think of the
setsid( ) call as the "new" way to do thing (disassociate
from the terminal) and the [second] fork( ) call after it
as redundancy to deal with the SVr4..."

[]
>>>> +             * stdout (temporarily) to the pipe to parent,
>>>
>>> This is a bit of a hack.
>>
>> There's another way -- to keep the writing pipe end in some
>> local variable and use that one instead of STDOUT_FILENO.
>> I can do it that way for sure, just thought it's already
>> using too much local variables.
> 
> Yes, that would be better.

Done in a v2 version I sent you.

>>>> +    /* now complete the daemonizing procedure.
>>>> +     */
>>>> +    if (device&&  !verbose) {
>>>> +        if (chdir("/")<  0) {
>>>> +            err(EXIT_FAILURE, "unable to chdir to /");
>>>> +        }
>>>> +        /* this redirects stderr to /dev/null */
>>>> +        dup2(STDIN_FILENO, STDERR_FILENO);
>>>> +        /* this redirects stdout to /dev/null too, and closes parent pipe */
>>>> +        dup2(STDIN_FILENO, STDOUT_FILENO);
>>>> +    }
>>>> +
>>>
>>> Half of this is already done in client_thread, and that would be
>>> theplace where you should add dup2(0, 1).

Um, I missed that "half of this" part.  Indeed, nbd_client_thread()
does dup2(STDOUT_FILENO, STDERR_FILENO) which should go away, but
it is harmless for now, and can be addressed in a separate patch.

>> I partly disagree.
>>
>> I wanted to de-couple -c (device) case with daemonizing.
>> client_thread only works in -c case, but daemonizing in
>> that case is wrong as I already pointed out in another
>> email - we should either stop daemonizing here at all
>> or have a separate option for it.
> 
> We can only clean up standard file descriptors after all initialization tasks have been done.  nbd_client_thread could still write error messages.  Your patch introduces a race.

Please elaborate where the race is.  Do you mean one
thread can write error message while another at the
same time is closing the filedescriptor in question, --
that race?  We're doomed anyway, and it is even good
we've a small remote chance for our error message to
be seen.  Currently it just goes to /dev/null.

>>>   Also, the chdir can be moved earlier, after bdrv_open.
>>
>> There's no need to, afiacs.  We complete init process and
>> enter main loop.  Chdir should be done befor entering main
>> loop, the rest makes no difference (as long as the files
>> we open will be accessible from cwd).
> 
> Yes, but I prefer to have the chdir done unconditionally as soon as possible.

That's not a bad intention.  I'm fixing existing logic without
introducing new logical changes.  If you want to fix other
stuff, it is better be done in a separate commit/change.

Thanks,

/mjt

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

* Re: [Qemu-devel] [PATCH] rework daemonizing logic in qemu-nbd
  2012-01-15 16:44       ` Michael Tokarev
@ 2012-01-15 17:31         ` Paolo Bonzini
  2012-01-15 17:46           ` Paolo Bonzini
  2012-01-16  7:22           ` Michael Tokarev
  0 siblings, 2 replies; 9+ messages in thread
From: Paolo Bonzini @ 2012-01-15 17:31 UTC (permalink / raw)
  To: Michael Tokarev; +Cc: qemu-devel

On 01/15/2012 05:44 PM, Michael Tokarev wrote:
>>>>> +             * stdout (temporarily) to the pipe to parent,
>>>>
>>>> This is a bit of a hack.
>>>
>>> There's another way -- to keep the writing pipe end in some
>>> local variable and use that one instead of STDOUT_FILENO.
>>> I can do it that way for sure, just thought it's already
>>> using too much local variables.
>>
>> Yes, that would be better.
>
> Done in a v2 version I sent you.

Please stay on the list.

>>>>> +    /* now complete the daemonizing procedure.
>>>>> +     */
>>>>> +    if (device&&   !verbose) {
>>>>> +        if (chdir("/")<   0) {
>>>>> +            err(EXIT_FAILURE, "unable to chdir to /");
>>>>> +        }
>>>>> +        /* this redirects stderr to /dev/null */
>>>>> +        dup2(STDIN_FILENO, STDERR_FILENO);
>>>>> +        /* this redirects stdout to /dev/null too, and closes parent pipe */
>>>>> +        dup2(STDIN_FILENO, STDOUT_FILENO);
>>>>> +    }
>>>>> +
>>>>
>>>> Half of this is already done in client_thread, and that would be
>>>> theplace where you should add dup2(0, 1).
>
> Um, I missed that "half of this" part.  Indeed, nbd_client_thread()
> does dup2(STDOUT_FILENO, STDERR_FILENO) which should go away, but
> it is harmless for now, and can be addressed in a separate patch.

Again, _the client thread_ is the right place to do this!  See below.

>>> I partly disagree.
>>>
>>> I wanted to de-couple -c (device) case with daemonizing.
>>> client_thread only works in -c case, but daemonizing in
>>> that case is wrong as I already pointed out in another
>>> email - we should either stop daemonizing here at all
>>> or have a separate option for it.
>>
>> We can only clean up standard file descriptors after all
>> initialization tasks have been done. nbd_client_thread could still write
>> error messages. Your patch introduces a race.
>
> Please elaborate where the race is.  Do you mean one
> thread can write error message while another at the
> same time is closing the filedescriptor in question, --
> that race?

Yes.

> We're doomed anyway, and it is even good
> we've a small remote chance for our error message to
> be seen.  Currently it just goes to /dev/null.

No, currently it is sent from the daemon to the parent through the pipe, 
the parent prints it and exits with status code 1.  With your patch, if 
the dup2 wins the race you exit with status code 0; if the client thread 
wins the race it is the same as master.

> That's not a bad intention.  I'm fixing existing logic without
> introducing new logical changes.  If you want to fix other
> stuff, it is better be done in a separate commit/change.

AFAIK the only known bug (besides the devfd/sockfd mixup) is the missing 
chdir, and that should be fixed first.

Paolo

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

* Re: [Qemu-devel] [PATCH] rework daemonizing logic in qemu-nbd
  2012-01-15 17:31         ` Paolo Bonzini
@ 2012-01-15 17:46           ` Paolo Bonzini
  2012-01-16  7:22           ` Michael Tokarev
  1 sibling, 0 replies; 9+ messages in thread
From: Paolo Bonzini @ 2012-01-15 17:46 UTC (permalink / raw)
  To: qemu-devel

On 01/15/2012 06:31 PM, Paolo Bonzini wrote:
>
>
>> We're doomed anyway, and it is even good
>> we've a small remote chance for our error message to
>> be seen.  Currently it just goes to /dev/null.
>
> No, currently it is sent from the daemon to the parent through the pipe,
> the parent prints it and exits with status code 1.  With your patch, if
> the dup2 wins the race you exit with status code 0; if the client thread
> wins the race it is the same as master.

Actually, the dup2 will always win the race.  Until the main loop starts 
and accepts the connection from the client thread, the client thread 
will be stuck connect()ing to the server socket.  So, the client thread 
will never be able to report problems connecting /dev/nbd (for example 
you won't get an error if you chose a device that is already busy).  So 
it looks like there is no race, but there is a bug. :)

Please disprove me if I'm wrong, of course.

Paolo

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

* Re: [Qemu-devel] [PATCH] rework daemonizing logic in qemu-nbd
  2012-01-15 17:31         ` Paolo Bonzini
  2012-01-15 17:46           ` Paolo Bonzini
@ 2012-01-16  7:22           ` Michael Tokarev
  2012-01-16  7:41             ` Paolo Bonzini
  1 sibling, 1 reply; 9+ messages in thread
From: Michael Tokarev @ 2012-01-16  7:22 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel

On 15.01.2012 21:31, Paolo Bonzini wrote:
> On 01/15/2012 05:44 PM, Michael Tokarev wrote:
>>>>>> +             * stdout (temporarily) to the pipe to parent,
>>>>>
>>>>> This is a bit of a hack.
>>>>
>>>> There's another way -- to keep the writing pipe end in some
>>>> local variable and use that one instead of STDOUT_FILENO.
>>>> I can do it that way for sure, just thought it's already
>>>> using too much local variables.
>>>
>>> Yes, that would be better.
>>
>> Done in a v2 version I sent you.
> 
> Please stay on the list.

Sorry?  I sent it to you and to the list, here's the command
line from my .bash_history:

 git format-patch --subject-prefix="PATCH v2" --stdout --to 'qemu-devel@nongnu.org' --cc "Paolo Bonzini <pbonzini@redhat.com>" --cc mjt@tls.msk.ru HEAD^ | /usr/sbin/sendmail -t -i

On which list I shoult stay?

[]
>> Um, I missed that "half of this" part.  Indeed, nbd_client_thread()
>> does dup2(STDOUT_FILENO, STDERR_FILENO) which should go away, but
>> it is harmless for now, and can be addressed in a separate patch.
> 
> Again, _the client thread_ is the right place to do this!  See below.

[]
>> We're doomed anyway, and it is even good
>> we've a small remote chance for our error message to
>> be seen.  Currently it just goes to /dev/null.
> 
> No, currently it is sent from the daemon to the parent through the pipe, the parent prints it and exits with status code 1.  With your patch, if the dup2 wins the race you exit with status code 0; if the client thread wins the race it is the same as master.

Aha. I finally see what you mean.

I still disagree, -- all the operations done in the client
thread can be done before forking a new thread, syncronously,
and _that_ will be the easiest and cleanest solution here.

>> That's not a bad intention.  I'm fixing existing logic without
>> introducing new logical changes.  If you want to fix other
>> stuff, it is better be done in a separate commit/change.
> 
> AFAIK the only known bug (besides the devfd/sockfd mixup) is the missing chdir, and that should be fixed first.

It all looked so ugly to me that I didn't even want to think
about just adding a chdir() instead of getting rid of daemon().
But ok, I can go that ugly route too.

Thanks,

/mjt

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

* Re: [Qemu-devel] [PATCH] rework daemonizing logic in qemu-nbd
  2012-01-16  7:22           ` Michael Tokarev
@ 2012-01-16  7:41             ` Paolo Bonzini
  0 siblings, 0 replies; 9+ messages in thread
From: Paolo Bonzini @ 2012-01-16  7:41 UTC (permalink / raw)
  To: Michael Tokarev; +Cc: qemu-devel

On 01/16/2012 08:22 AM, Michael Tokarev wrote:
> On 15.01.2012 21:31, Paolo Bonzini wrote:
>> On 01/15/2012 05:44 PM, Michael Tokarev wrote:
>>>>>>> +             * stdout (temporarily) to the pipe to parent,
>>>>>>
>>>>>> This is a bit of a hack.
>>>>>
>>>>> There's another way -- to keep the writing pipe end in some
>>>>> local variable and use that one instead of STDOUT_FILENO.
>>>>> I can do it that way for sure, just thought it's already
>>>>> using too much local variables.
>>>>
>>>> Yes, that would be better.
>>>
>>> Done in a v2 version I sent you.
>>
>> Please stay on the list.
>
> Sorry?  I sent it to you and to the list, here's the command
> line from my .bash_history:
>
>   git format-patch --subject-prefix="PATCH v2" --stdout --to 'qemu-devel@nongnu.org' --cc "Paolo Bonzini<pbonzini@redhat.com>" --cc mjt@tls.msk.ru HEAD^ | /usr/sbin/sendmail -t -i
>
> On which list I shoult stay?

Ah, sorry, I don't connect to the VPN usually during the weekend so I 
thought it was sent privately.

> I still disagree, -- all the operations done in the client
> thread can be done before forking a new thread, syncronously,
> and _that_ will be the easiest and cleanest solution here.

It's a chicken-and-egg problem.  To connect a socket to an NBD device, 
you need to have negotiated with the server already, so the socket must 
be connected already.  Since the other side of the socket is also 
handled by qemu-nbd, you need threads.

It's true that in principle you could use the main loop and do the 
NBD_DO_IT in a new thread.  However, qemu-sockets.c does not support 
asynchronous connect(2).  You could run that code already inside the 
main loop, for example in a coroutine, but then patches pile up quickly.

>>> That's not a bad intention.  I'm fixing existing logic without
>>> introducing new logical changes.  If you want to fix other
>>> stuff, it is better be done in a separate commit/change.
>>
>> AFAIK the only known bug (besides the devfd/sockfd mixup) is the
>> missing chdir, and that should be fixed first.
>
> It all looked so ugly to me that I didn't even want to think
> about just adding a chdir() instead of getting rid of daemon().
> But ok, I can go that ugly route too.

I'm not saying it's particularly pretty, but it's also not as easy to 
improve as you'd think.  There are a lot of constraints (fork/daemonize 
before initializing the block layer, report errors properly, avoid 
races, ...).

Paolo

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

end of thread, other threads:[~2012-01-16  7:41 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-01-14 12:39 [Qemu-devel] [PATCH] rework daemonizing logic in qemu-nbd Michael Tokarev
2012-01-15 10:42 ` Paolo Bonzini
2012-01-15 12:50   ` Michael Tokarev
2012-01-15 16:11     ` Paolo Bonzini
2012-01-15 16:44       ` Michael Tokarev
2012-01-15 17:31         ` Paolo Bonzini
2012-01-15 17:46           ` Paolo Bonzini
2012-01-16  7:22           ` Michael Tokarev
2012-01-16  7:41             ` Paolo Bonzini

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.