Message ID | 1351343715-7446-1-git-send-email-mjt@msgid.tls.msk.ru |
---|---|
State | New |
Headers | show |
On Sat, Oct 27, 2012 at 05:15:15PM +0400, Michael Tokarev wrote: > diff --git a/vl.c b/vl.c > index 9f99ef4..db48d62 100644 > --- a/vl.c > +++ b/vl.c > @@ -3413,6 +3413,26 @@ int main(int argc, char **argv, char **envp) > default_sdcard = 0; > } > > + if (is_daemonized()) { > + /* According to documentation and historically, -nographic redirects > + * serial port, parallel port and monitor to stdio, which does not work > + * with -daemonize. We can redirect these to null instead, but since > + * -nographic is legacy, let's just error out. > + */ > + if (display_type == DT_NOGRAPHIC > + /* && (default_parallel || default_serial > + || default_monitor || default_virtcon) */) { Uncomment these? Stefan
29.10.2012 13:18, Stefan Hajnoczi wrote: > On Sat, Oct 27, 2012 at 05:15:15PM +0400, Michael Tokarev wrote: >> diff --git a/vl.c b/vl.c >> index 9f99ef4..db48d62 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -3413,6 +3413,26 @@ int main(int argc, char **argv, char **envp) >> default_sdcard = 0; >> } >> >> + if (is_daemonized()) { >> + /* According to documentation and historically, -nographic redirects >> + * serial port, parallel port and monitor to stdio, which does not work >> + * with -daemonize. We can redirect these to null instead, but since >> + * -nographic is legacy, let's just error out. >> + */ >> + if (display_type == DT_NOGRAPHIC >> + /* && (default_parallel || default_serial >> + || default_monitor || default_virtcon) */) { > > Uncomment these? I'd say treat it as a documentation comment, sort of. If all 4 other options are specified, -nographics has no effect, so this very case is not very interesting -- once you specify all 4, you don't need -nographic. But keeping this special case around makes behavour less consistent: -nographic starts sometimes working and sometimes not. Now when it isn't possible to use stdio chr backend with -daemonize, it isn't really necessary to test even for DT_NOGRAPHIC: if we wont, next we'll try to create stdio backend which will fail. The only purpose for this test is to give more understandable error message. (Checking for DT_CURSES is still necessary). Thanks, /mjt
On Mon, Oct 29, 2012 at 04:26:58PM +0400, Michael Tokarev wrote: > 29.10.2012 13:18, Stefan Hajnoczi wrote: > > On Sat, Oct 27, 2012 at 05:15:15PM +0400, Michael Tokarev wrote: > >> diff --git a/vl.c b/vl.c > >> index 9f99ef4..db48d62 100644 > >> --- a/vl.c > >> +++ b/vl.c > >> @@ -3413,6 +3413,26 @@ int main(int argc, char **argv, char **envp) > >> default_sdcard = 0; > >> } > >> > >> + if (is_daemonized()) { > >> + /* According to documentation and historically, -nographic redirects > >> + * serial port, parallel port and monitor to stdio, which does not work > >> + * with -daemonize. We can redirect these to null instead, but since > >> + * -nographic is legacy, let's just error out. > >> + */ > >> + if (display_type == DT_NOGRAPHIC > >> + /* && (default_parallel || default_serial > >> + || default_monitor || default_virtcon) */) { > > > > Uncomment these? > > I'd say treat it as a documentation comment, sort of. > If all 4 other options are specified, -nographics has > no effect, so this very case is not very interesting -- > once you specify all 4, you don't need -nographic. > But keeping this special case around makes behavour > less consistent: -nographic starts sometimes working > and sometimes not. In that case I suggest we remove it or replace it with a comment in English. Commented out code usually confuses me more than helps, as this example shows :). Stefan
diff --git a/qemu-char.c b/qemu-char.c index afe2bfb..21c6a0d 100644 --- a/qemu-char.c +++ b/qemu-char.c @@ -772,6 +772,10 @@ static CharDriverState *qemu_chr_open_stdio(QemuOpts *opts) if (stdio_nb_clients >= STDIO_MAX_CLIENTS) { return NULL; } + if (is_daemonized()) { + error_report("cannot use stdio with -daemonize"); + return NULL; + } if (stdio_nb_clients == 0) { old_fd0_flags = fcntl(0, F_GETFL); tcgetattr (0, &oldtty); diff --git a/vl.c b/vl.c index 9f99ef4..db48d62 100644 --- a/vl.c +++ b/vl.c @@ -3413,6 +3413,26 @@ int main(int argc, char **argv, char **envp) default_sdcard = 0; } + if (is_daemonized()) { + /* According to documentation and historically, -nographic redirects + * serial port, parallel port and monitor to stdio, which does not work + * with -daemonize. We can redirect these to null instead, but since + * -nographic is legacy, let's just error out. + */ + if (display_type == DT_NOGRAPHIC + /* && (default_parallel || default_serial + || default_monitor || default_virtcon) */) { + fprintf(stderr, "-nographic can not be used with -daemonize\n"); + exit(1); + } +#ifdef CONFIG_CURSES + if (display_type == DT_CURSES) { + fprintf(stderr, "curses display can not be used with -daemonize\n"); + exit(1); + } +#endif + } + if (display_type == DT_NOGRAPHIC) { if (default_parallel) add_device_config(DEV_PARALLEL, "null"); @@ -3687,9 +3707,7 @@ int main(int argc, char **argv, char **envp) break; #if defined(CONFIG_CURSES) case DT_CURSES: - if (!is_daemonized()) { - curses_display_init(ds, full_screen); - } + curses_display_init(ds, full_screen); break; #endif #if defined(CONFIG_SDL)
Curses display requires stdin/out to stay on the terminal, so -daemonize makes no sense in this case. Instead of leaving display uninitialized like is done since 995ee2bf469de6bb, explicitly detect this case earlier and error out. -nographic can actually be used with -daemonize, by redirecting everything to a null device, but the problem is that according to documentation and historical behavour, -nographic redirects guest ports to stdin/out, which, again, makes no sense in case of -daemonize. Since -nographic is a legacy option, don't bother fixing this case (to allow -nographic and -daemonize by redirecting guest ports to null instead of stdin/out in this case), but disallow it completely instead, to stop garbling host terminal. If no display display needed and user wants to use -nographic, the right way to go is to use -serial null -parallel null -monitor none -display none -vga none instead of -nographic. Also prevent the same issue -- it was possible to get garbled host tty after -nographic -daemonize and it is still possible to have it by using -serial stdio -daemonize Fix this by disallowing opening stdio chardev when -daemonize is specified. Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> --- qemu-char.c | 4 ++++ vl.c | 24 +++++++++++++++++++++--- 2 files changed, 25 insertions(+), 3 deletions(-)