[Qemu-devel] [PATCH] disallow -daemonize usage of stdio (curses display, -nographic, -serial stdio etc)

[Qemu-devel] [PATCH] disallow -daemonize usage of stdio (curses display, -nographic, -serial stdio etc)

[Michael Tokarev]

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(-)

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)
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH] disallow -daemonize usage of stdio (curses display, -nographic, -serial stdio etc)

[Stefan Hajnoczi]

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

Re: [Qemu-devel] [PATCH] disallow -daemonize usage of stdio (curses display, -nographic, -serial stdio etc)

[Michael Tokarev]

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

Re: [Qemu-devel] [PATCH] disallow -daemonize usage of stdio (curses display, -nographic, -serial stdio etc)

[Stefan Hajnoczi]

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