| Message ID | 1472142645-7370-2-git-send-email-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Dear Max, thanks for taking the time to fix the race condition! Max Reitz <mreitz@redhat.com> writes: > Using the --fork option, one can make qemu-nbd fork the worker process. > The original process will exit on error of the worker or once the worker > enters the main loop. > @@ -773,7 +780,7 @@ int main(int argc, char **argv) > return 0; > } > > - if (device && !verbose) { > + if ((device && !verbose) || fork_process) { > int stderr_fd[2]; > pid_t pid; > int ret; Looking at the surrounding (unchanged) code I see that qemu-nbd already implemented a daemon mode. It's just that it's completely undocumented and hinges on both the --device and the --verbose option. Yuck. It seems there are two things --verbose does (from a user point of view): 1. Print "NBD device %s is now connected to %s" and keep stderr open. Debug messages are always printed to stderr, but in non-verbose daemon mode they end up at /dev/null. This is more or less what one usually expects from an option named --verbose. Except that it only affects daemon mode and messages are always printed (but end up at /dev/null). 2. Disable daemon mode. I might expect this for an option named --debug, but certainly not for --verbose... A clean way forward would be something like this: 1. Introduce --foreground / --daemon, --quiet Default to daemon mode with silent output if --connect is given, foreground mode with visible output otherwise. Set non-daemon mode with visible output if --verbose is given. Let --foreground / --daemon / --quiet any default or implicit value. Document that --verbose implicitly enables daemon mode for compatibility with previous versions and that future versions may stop doing so (i.e. users should use either --verbose --foreground or --verbose --daemon). 3. At some point in the future (qemu 3.0?) we can stop having --verbose imply --foreground. I can give it a try if it's out of scope for your current task. Sascha
On 29.08.2016 18:59, Sascha Silbe wrote: > Dear Max, > > > thanks for taking the time to fix the race condition! > > > Max Reitz <mreitz@redhat.com> writes: > >> Using the --fork option, one can make qemu-nbd fork the worker process. >> The original process will exit on error of the worker or once the worker >> enters the main loop. > >> @@ -773,7 +780,7 @@ int main(int argc, char **argv) >> return 0; >> } >> >> - if (device && !verbose) { >> + if ((device && !verbose) || fork_process) { >> int stderr_fd[2]; >> pid_t pid; >> int ret; > > Looking at the surrounding (unchanged) code I see that qemu-nbd already > implemented a daemon mode. It's just that it's completely undocumented > and hinges on both the --device and the --verbose option. Yuck. > > It seems there are two things --verbose does (from a user point of > view): > > 1. Print "NBD device %s is now connected to %s" and keep stderr open. > > Debug messages are always printed to stderr, but in non-verbose > daemon mode they end up at /dev/null. > > This is more or less what one usually expects from an option named > --verbose. Except that it only affects daemon mode and messages are > always printed (but end up at /dev/null). > > 2. Disable daemon mode. > > I might expect this for an option named --debug, but certainly not > for --verbose... Does it? There is explicit daemon mode until this patch. While it is actually implemented, it is only used implicitly when you want to connect to the kernel's NBD interface (which is what the --connect option is for (whose argument is kept in the @device variable)). This patch introduces a way to generally make use of the "daemon" mode; and when you use the respective option (--fork), then it will work regardless of whether you have specified --verbose or not. The only thing --verbose does is disable the implicit daemon mode when using --connect, and that seems reasonable to me. > A clean way forward would be something like this: > > 1. Introduce --foreground / --daemon, --quiet > > Default to daemon mode with silent output if --connect is given, > foreground mode with visible output otherwise. Set non-daemon mode > with visible output if --verbose is given. Let --foreground / > --daemon / --quiet any default or implicit value. Document that > --verbose implicitly enables daemon mode for compatibility with > previous versions and that future versions may stop doing so > (i.e. users should use either --verbose --foreground or --verbose > --daemon). Note that we haven't even documented that --connect implicitly puts qemu-nbd in a daemon mode. > 3. At some point in the future (qemu 3.0?) we can stop having --verbose > imply --foreground. While in my opinion --verbose actually is a debugging option, I don't think the behavior of --connect when used together with --verbose really affects this patch series. > I can give it a try if it's out of scope for your current task. I certainly won't stop you. ;-) Max
Am 25.08.2016 um 18:30 hat Max Reitz geschrieben: > Using the --fork option, one can make qemu-nbd fork the worker process. > The original process will exit on error of the worker or once the worker > enters the main loop. > > Suggested-by: Sascha Silbe <silbe@linux.vnet.ibm.com> > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > qemu-nbd.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/qemu-nbd.c b/qemu-nbd.c > index e3571c2..8c2d582 100644 > --- a/qemu-nbd.c > +++ b/qemu-nbd.c > @@ -48,6 +48,7 @@ > #define QEMU_NBD_OPT_OBJECT 260 > #define QEMU_NBD_OPT_TLSCREDS 261 > #define QEMU_NBD_OPT_IMAGE_OPTS 262 > +#define QEMU_NBD_OPT_FORK 263 > > #define MBR_SIZE 512 > > @@ -503,6 +504,7 @@ int main(int argc, char **argv) > { "tls-creds", required_argument, NULL, QEMU_NBD_OPT_TLSCREDS }, > { "image-opts", no_argument, NULL, QEMU_NBD_OPT_IMAGE_OPTS }, > { "trace", required_argument, NULL, 'T' }, > + { "fork", no_argument, NULL, QEMU_NBD_OPT_FORK }, > { NULL, 0, NULL, 0 } > }; > int ch; > @@ -524,6 +526,8 @@ int main(int argc, char **argv) > bool imageOpts = false; > bool writethrough = true; > char *trace_file = NULL; > + bool fork_process = false; > + int old_stderr = -1; > > /* The client thread uses SIGTERM to interrupt the server. A signal > * handler ensures that "qemu-nbd -v -c" exits with a nice status code. > @@ -714,6 +718,9 @@ int main(int argc, char **argv) > g_free(trace_file); > trace_file = trace_opt_parse(optarg); > break; > + case QEMU_NBD_OPT_FORK: > + fork_process = true; > + break; > } > } > > @@ -773,7 +780,7 @@ int main(int argc, char **argv) > return 0; > } > > - if (device && !verbose) { > + if ((device && !verbose) || fork_process) { > int stderr_fd[2]; > pid_t pid; > int ret; > @@ -796,6 +803,7 @@ int main(int argc, char **argv) > ret = qemu_daemon(1, 0); > > /* Temporarily redirect stderr to the parent's pipe... */ > + old_stderr = dup(STDERR_FILENO); > dup2(stderr_fd[1], STDERR_FILENO); > if (ret < 0) { > error_report("Failed to daemonize: %s", strerror(errno)); I don't have a kernel with NBD support, so I can't test this easily, but doesn't this break the daemon mode for --connect? I mean, it will still fork, but won't the parent be alive until the child exits? > @@ -951,6 +959,11 @@ int main(int argc, char **argv) > exit(EXIT_FAILURE); > } > > + if (fork_process) { > + dup2(old_stderr, STDERR_FILENO); > + close(old_stderr); > + } Because this code doesn't run for --connect (unless --fork is given, too). > state = RUNNING; > do { > main_loop_wait(false); The documentation needs an update, too. Kevin
On 27.09.2016 11:04, Kevin Wolf wrote: > Am 25.08.2016 um 18:30 hat Max Reitz geschrieben: >> Using the --fork option, one can make qemu-nbd fork the worker process. >> The original process will exit on error of the worker or once the worker >> enters the main loop. >> >> Suggested-by: Sascha Silbe <silbe@linux.vnet.ibm.com> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> qemu-nbd.c | 15 ++++++++++++++- >> 1 file changed, 14 insertions(+), 1 deletion(-) >> >> diff --git a/qemu-nbd.c b/qemu-nbd.c >> index e3571c2..8c2d582 100644 >> --- a/qemu-nbd.c >> +++ b/qemu-nbd.c >> @@ -48,6 +48,7 @@ >> #define QEMU_NBD_OPT_OBJECT 260 >> #define QEMU_NBD_OPT_TLSCREDS 261 >> #define QEMU_NBD_OPT_IMAGE_OPTS 262 >> +#define QEMU_NBD_OPT_FORK 263 >> >> #define MBR_SIZE 512 >> >> @@ -503,6 +504,7 @@ int main(int argc, char **argv) >> { "tls-creds", required_argument, NULL, QEMU_NBD_OPT_TLSCREDS }, >> { "image-opts", no_argument, NULL, QEMU_NBD_OPT_IMAGE_OPTS }, >> { "trace", required_argument, NULL, 'T' }, >> + { "fork", no_argument, NULL, QEMU_NBD_OPT_FORK }, >> { NULL, 0, NULL, 0 } >> }; >> int ch; >> @@ -524,6 +526,8 @@ int main(int argc, char **argv) >> bool imageOpts = false; >> bool writethrough = true; >> char *trace_file = NULL; >> + bool fork_process = false; >> + int old_stderr = -1; >> >> /* The client thread uses SIGTERM to interrupt the server. A signal >> * handler ensures that "qemu-nbd -v -c" exits with a nice status code. >> @@ -714,6 +718,9 @@ int main(int argc, char **argv) >> g_free(trace_file); >> trace_file = trace_opt_parse(optarg); >> break; >> + case QEMU_NBD_OPT_FORK: >> + fork_process = true; >> + break; >> } >> } >> >> @@ -773,7 +780,7 @@ int main(int argc, char **argv) >> return 0; >> } >> >> - if (device && !verbose) { >> + if ((device && !verbose) || fork_process) { >> int stderr_fd[2]; >> pid_t pid; >> int ret; >> @@ -796,6 +803,7 @@ int main(int argc, char **argv) >> ret = qemu_daemon(1, 0); >> >> /* Temporarily redirect stderr to the parent's pipe... */ >> + old_stderr = dup(STDERR_FILENO); >> dup2(stderr_fd[1], STDERR_FILENO); >> if (ret < 0) { >> error_report("Failed to daemonize: %s", strerror(errno)); > > I don't have a kernel with NBD support, so I can't test this easily, but > doesn't this break the daemon mode for --connect? I mean, it will still > fork, but won't the parent be alive until the child exits? Well, for me the parent closes as it should. > >> @@ -951,6 +959,11 @@ int main(int argc, char **argv) >> exit(EXIT_FAILURE); >> } >> >> + if (fork_process) { >> + dup2(old_stderr, STDERR_FILENO); >> + close(old_stderr); >> + } > > Because this code doesn't run for --connect (unless --fork is given, > too). Hm, so? It never ran before either, because I'm only just now introducing it. And the fact that I'm keeping the original stderr FD open has nothing to do with when the parent process will quit because the parent process is not connected to that *original* stderr. Also, when using --connect, the FD is closed in nbd_client_thread(). > >> state = RUNNING; >> do { >> main_loop_wait(false); > > The documentation needs an update, too. Right. I wonder why I forgot this. I guess the answer is "Because I wrote this in some spare time at KVM Forum to see if it would work at all"... Max
diff --git a/qemu-nbd.c b/qemu-nbd.c index e3571c2..8c2d582 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -48,6 +48,7 @@ #define QEMU_NBD_OPT_OBJECT 260 #define QEMU_NBD_OPT_TLSCREDS 261 #define QEMU_NBD_OPT_IMAGE_OPTS 262 +#define QEMU_NBD_OPT_FORK 263 #define MBR_SIZE 512 @@ -503,6 +504,7 @@ int main(int argc, char **argv) { "tls-creds", required_argument, NULL, QEMU_NBD_OPT_TLSCREDS }, { "image-opts", no_argument, NULL, QEMU_NBD_OPT_IMAGE_OPTS }, { "trace", required_argument, NULL, 'T' }, + { "fork", no_argument, NULL, QEMU_NBD_OPT_FORK }, { NULL, 0, NULL, 0 } }; int ch; @@ -524,6 +526,8 @@ int main(int argc, char **argv) bool imageOpts = false; bool writethrough = true; char *trace_file = NULL; + bool fork_process = false; + int old_stderr = -1; /* The client thread uses SIGTERM to interrupt the server. A signal * handler ensures that "qemu-nbd -v -c" exits with a nice status code. @@ -714,6 +718,9 @@ int main(int argc, char **argv) g_free(trace_file); trace_file = trace_opt_parse(optarg); break; + case QEMU_NBD_OPT_FORK: + fork_process = true; + break; } } @@ -773,7 +780,7 @@ int main(int argc, char **argv) return 0; } - if (device && !verbose) { + if ((device && !verbose) || fork_process) { int stderr_fd[2]; pid_t pid; int ret; @@ -796,6 +803,7 @@ int main(int argc, char **argv) ret = qemu_daemon(1, 0); /* Temporarily redirect stderr to the parent's pipe... */ + old_stderr = dup(STDERR_FILENO); dup2(stderr_fd[1], STDERR_FILENO); if (ret < 0) { error_report("Failed to daemonize: %s", strerror(errno)); @@ -951,6 +959,11 @@ int main(int argc, char **argv) exit(EXIT_FAILURE); } + if (fork_process) { + dup2(old_stderr, STDERR_FILENO); + close(old_stderr); + } + state = RUNNING; do { main_loop_wait(false);
Using the --fork option, one can make qemu-nbd fork the worker process. The original process will exit on error of the worker or once the worker enters the main loop. Suggested-by: Sascha Silbe <silbe@linux.vnet.ibm.com> Signed-off-by: Max Reitz <mreitz@redhat.com> --- qemu-nbd.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)