Patchwork [v2] stop using stdio for monitor/serial/etc with -daemonize

login
register
mail settings
Submitter Michael Tokarev
Date Sept. 25, 2012, 2:48 p.m.
Message ID <1348584532-21914-1-git-send-email-mjt@msgid.tls.msk.ru>
Download mbox | patch
Permalink /patch/186834/
State New
Headers show

Comments

Michael Tokarev - Sept. 25, 2012, 2:48 p.m.
Current code binds monitor and serial port to the guest console
unless -nographic is specified, which is okay.  But when there's
no guest console (-nographic), the code tries to use stdio for
the same default devices.  But it does not check for -daemonize
at the same time -- because when -daemonize is given, there's no
point at using stdin since it will be closed down the line.
However, when serial port is attached to stdin, tty control
modes are changed (switching tty to raw mode), and qemu will
switch to background, leaving the tty in bad state.

Take -daemonize into account too, when assigning default devices,
and for -nographic -daemonize case, assign them to "null" instead.

This is https://bugs.launchpad.net/qemu/+bug/1024275
or http://bugs.debian.org/549195 .

While at it, reformat this code a bit to be less of a maze of
ifs and use a variable to hold common target of devices.

This patch depends on another patch, 995ee2bf469de6,
"curses: don't initialize curses when qemu is daemonized",
by Hitoshi Mitake, which creates is_daemonized() routine.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 vl.c |   45 +++++++++++++++++++++++++++------------------
 1 file changed, 27 insertions(+), 18 deletions(-)
Anthony Liguori - Sept. 25, 2012, 9:19 p.m.
Michael Tokarev <mjt@tls.msk.ru> writes:

> Current code binds monitor and serial port to the guest console
> unless -nographic is specified, which is okay.  But when there's
> no guest console (-nographic), the code tries to use stdio for
> the same default devices.  But it does not check for -daemonize
> at the same time -- because when -daemonize is given, there's no
> point at using stdin since it will be closed down the line.
> However, when serial port is attached to stdin, tty control
> modes are changed (switching tty to raw mode), and qemu will
> switch to background, leaving the tty in bad state.
>
> Take -daemonize into account too, when assigning default devices,
> and for -nographic -daemonize case, assign them to "null" instead.

Combining -nographic and -daemonize don't make sense.  I'd rather error
out with this combination.

I think what the user is after is -daemonize -vga none OR -daemonize
-display none.

Regards,

Anthony Liguori

>
> This is https://bugs.launchpad.net/qemu/+bug/1024275
> or http://bugs.debian.org/549195 .
>
> While at it, reformat this code a bit to be less of a maze of
> ifs and use a variable to hold common target of devices.
>
> This patch depends on another patch, 995ee2bf469de6,
> "curses: don't initialize curses when qemu is daemonized",
> by Hitoshi Mitake, which creates is_daemonized() routine.
>
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  vl.c |   45 +++++++++++++++++++++++++++------------------
>  1 file changed, 27 insertions(+), 18 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 48049ef..a210ff9 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3392,30 +3392,39 @@ int main(int argc, char **argv, char **envp)
>          default_sdcard = 0;
>      }
>  
> -    if (display_type == DT_NOGRAPHIC) {
> -        if (default_parallel)
> -            add_device_config(DEV_PARALLEL, "null");
> +    /* Create default monitor, serial, parallel and virtcon devices. */
> +    /* reuse optarg variable */
> +    optarg = NULL;
> +    if (display_type != DT_NOGRAPHIC) {
> +        /* regular case, all devices directed to the guest console */
> +        optarg = "vc:80Cx24C";
> +    } else if (is_daemonized()) {
> +        /* nographic and daemonize, everything => null */
> +        optarg = "null";
> +    } else {
> +        /* nographic and no daemonize */
> +        /* can't have both serial and virtcon on stdio */
>          if (default_serial && default_monitor) {
>              add_device_config(DEV_SERIAL, "mon:stdio");
>          } else if (default_virtcon && default_monitor) {
>              add_device_config(DEV_VIRTCON, "mon:stdio");
>          } else {
> -            if (default_serial)
> -                add_device_config(DEV_SERIAL, "stdio");
> -            if (default_virtcon)
> -                add_device_config(DEV_VIRTCON, "stdio");
> -            if (default_monitor)
> -                monitor_parse("stdio", "readline");
> +            optarg = "stdio";
>          }
> -    } else {
> -        if (default_serial)
> -            add_device_config(DEV_SERIAL, "vc:80Cx24C");
> -        if (default_parallel)
> -            add_device_config(DEV_PARALLEL, "vc:80Cx24C");
> -        if (default_monitor)
> -            monitor_parse("vc:80Cx24C", "readline");
> -        if (default_virtcon)
> -            add_device_config(DEV_VIRTCON, "vc:80Cx24C");
> +    }
> +    if (optarg && default_serial) {
> +        add_device_config(DEV_SERIAL, optarg);
> +    }
> +    if (default_parallel) {
> +        /* parallel port is connected console or to null */
> +        add_device_config(DEV_PARALLEL,
> +                          display_type == DT_NOGRAPHIC ? "null" : optarg);
> +    }
> +    if (optarg && default_monitor) {
> +        monitor_parse(optarg, "readline");
> +    }
> +    if (optarg && default_virtcon) {
> +        add_device_config(DEV_VIRTCON, optarg);
>      }
>  
>      socket_init();
> -- 
> 1.7.10.4
Michael Tokarev - Sept. 26, 2012, 7:09 a.m.
On 26.09.2012 01:19, Anthony Liguori wrote:
> Combining -nographic and -daemonize don't make sense.  I'd rather error
> out with this combination.
> 
> I think what the user is after is -daemonize -vga none OR -daemonize
> -display none.

So what's the difference?

I know lots of people use -nographic -daemonize to run headless
guests in background (like, for example, a router).  I guess it
come way before -vga option has been introduced, but at least I
know about -vga (but not about -vga none).  For one, I never saw
-display before.  And it looks like -nographic is a synonym for
-display none, and -curses is a synonym for -display curses.

It looks like we have way too many confusing options doing the
same thing.  And I think they should be consistent, at least
when they SMELL like they do the same thing, instead of forbidding
one or another in some situations.

Besides, the patch which I based my change on, "curses: don't initialize
curses when qemu is daemonized", probably makes no sense too, since
it is a situation with -curses -daemonize (or, -- is there a difference? --
-display curses -daemonize).  That situation is better be errored
out than worked around, I think.  (You just pulled that patch from
Stefanha).

Thanks,

/mjt
Peter Maydell - Sept. 26, 2012, 8 a.m.
On 26 September 2012 08:09, Michael Tokarev <mjt@tls.msk.ru> wrote:
> On 26.09.2012 01:19, Anthony Liguori wrote:
>> Combining -nographic and -daemonize don't make sense.  I'd rather error
>> out with this combination.
>>
>> I think what the user is after is -daemonize -vga none OR -daemonize
>> -display none.
>
> So what's the difference?
>
> I know lots of people use -nographic -daemonize to run headless
> guests in background (like, for example, a router).  I guess it
> come way before -vga option has been introduced, but at least I
> know about -vga (but not about -vga none).  For one, I never saw
> -display before.  And it looks like -nographic is a synonym for
> -display none, and -curses is a synonym for -display curses.

-nographic does about three different things at once (and I think
some of its effects aren't documented). It's a legacy option retained
for backward compatibility with old command lines.
If you want something that is non-confusing and makes sense, then
use -display none to disable graphics, -serial stdio to send serial
to stdio, and so on. These newer options do one clear thing each
and can be combined straightforwardly.

> It looks like we have way too many confusing options doing the
> same thing.  And I think they should be consistent, at least
> when they SMELL like they do the same thing, instead of forbidding
> one or another in some situations.

I'd love to drop -nographic but we'd break huge numbers of
existing setups...

-- PMM
Michael Tokarev - Sept. 26, 2012, 8:17 a.m.
On 26.09.2012 12:00, Peter Maydell wrote:

>> I know lots of people use -nographic -daemonize to run headless
>> guests in background (like, for example, a router).  I guess it
>> come way before -vga option has been introduced, but at least I
>> know about -vga (but not about -vga none).  For one, I never saw
>> -display before.  And it looks like -nographic is a synonym for
>> -display none, and -curses is a synonym for -display curses.

I mean, -nographic is about the same as -vga none -display none.

> -nographic does about three different things at once (and I think
> some of its effects aren't documented). It's a legacy option retained
> for backward compatibility with old command lines.

Sure.  Just like, for example, -stdvga was at the time being.

> If you want something that is non-confusing and makes sense, then
> use -display none to disable graphics, -serial stdio to send serial
> to stdio, and so on. These newer options do one clear thing each
> and can be combined straightforwardly.
> 
>> It looks like we have way too many confusing options doing the
>> same thing.  And I think they should be consistent, at least
>> when they SMELL like they do the same thing, instead of forbidding
>> one or another in some situations.
> 
> I'd love to drop -nographic but we'd break huge numbers of
> existing setups...

So let's make it actually work as expected till we're able to finally
drop it.

What is equivalent of -nographic in terms of -vga/-display/-...?
From the code it is something like

 -vga none -display none -serial mon:stdio -parallel null

(this is the code I tried to patch).

Note: this, compbined with -daemonize, also has the same issue,
namely, the tty is left in a bad state after qemu process backgrounded,
and for the very same reason: -serial stdio switches the try into
raw mode.  So this should be fixed too -- somehow, either by forbidding
this combination completely or by silently substituting stdio for
-serial with null.  But it will be done in a subsequent patch.

Note also: by forbidding -nographic -daemonize, we'll break lots of
existing setups too, and I still don't see why this combination is
bad, I already demonstrated that it can be made to work in a more
or less reasonable/expected way.

Thanks,

/mjt
Peter Maydell - Sept. 26, 2012, 8:43 a.m.
On 26 September 2012 09:17, Michael Tokarev <mjt@tls.msk.ru> wrote:
> On 26.09.2012 12:00, Peter Maydell wrote:
>
>>> I know lots of people use -nographic -daemonize to run headless
>>> guests in background (like, for example, a router).  I guess it
>>> come way before -vga option has been introduced, but at least I
>>> know about -vga (but not about -vga none).  For one, I never saw
>>> -display before.  And it looks like -nographic is a synonym for
>>> -display none, and -curses is a synonym for -display curses.
>
> I mean, -nographic is about the same as -vga none -display none.

...except that it *also* messes around with where the serial output
goes and with the parallel port and maybe something else.

> What is equivalent of -nographic in terms of -vga/-display/-...?
> From the code it is something like
>
>  -vga none -display none -serial mon:stdio -parallel null

It's something like that. It would be nice to implement -nographic
as "this is an alias for ...." but IIRC it isn't quite doable.
(maybe I misremember)

> (this is the code I tried to patch).
>
> Note: this, compbined with -daemonize, also has the same issue,
> namely, the tty is left in a bad state after qemu process backgrounded,
> and for the very same reason: -serial stdio switches the try into
> raw mode.  So this should be fixed too -- somehow, either by forbidding
> this combination completely or by silently substituting stdio for
> -serial with null.  But it will be done in a subsequent patch.
>
> Note also: by forbidding -nographic -daemonize, we'll break lots of
> existing setups too, and I still don't see why this combination is
> bad, I already demonstrated that it can be made to work in a more
> or less reasonable/expected way.

Because you've asked both "put me into the background" and "please
send stuff to stdio". Admittedly you've probably done that because
you didn't really understand that '-nographic' doesn't mean
'-display none', but you've still asked for a nonsensical combination.

-- PMM
Anthony Liguori - Sept. 26, 2012, 1:46 p.m.
Peter Maydell <peter.maydell@linaro.org> writes:

> On 26 September 2012 09:17, Michael Tokarev <mjt@tls.msk.ru> wrote:
>> On 26.09.2012 12:00, Peter Maydell wrote:
>>
>>>> I know lots of people use -nographic -daemonize to run headless
>>>> guests in background (like, for example, a router).  I guess it
>>>> come way before -vga option has been introduced, but at least I
>>>> know about -vga (but not about -vga none).  For one, I never saw
>>>> -display before.  And it looks like -nographic is a synonym for
>>>> -display none, and -curses is a synonym for -display curses.
>>
>> I mean, -nographic is about the same as -vga none -display none.
>
> ...except that it *also* messes around with where the serial output
> goes and with the parallel port and maybe something else.
>
>> What is equivalent of -nographic in terms of -vga/-display/-...?
>> From the code it is something like
>>
>>  -vga none -display none -serial mon:stdio -parallel null
>
> It's something like that. It would be nice to implement -nographic
> as "this is an alias for ...." but IIRC it isn't quite doable.
> (maybe I misremember)
>
>> (this is the code I tried to patch).
>>
>> Note: this, compbined with -daemonize, also has the same issue,
>> namely, the tty is left in a bad state after qemu process backgrounded,
>> and for the very same reason: -serial stdio switches the try into
>> raw mode.  So this should be fixed too -- somehow, either by forbidding
>> this combination completely or by silently substituting stdio for
>> -serial with null.  But it will be done in a subsequent patch.
>>
>> Note also: by forbidding -nographic -daemonize, we'll break lots of
>> existing setups too, and I still don't see why this combination is
>> bad, I already demonstrated that it can be made to work in a more
>> or less reasonable/expected way.
>
> Because you've asked both "put me into the background" and "please
> send stuff to stdio". Admittedly you've probably done that because
> you didn't really understand that '-nographic' doesn't mean
> '-display none', but you've still asked for a nonsensical combination.

This is a good example of where we need improved documentation but I
agree 100% with Peter.

Regards,

Anthony Liguori

>
> -- PMM
Michael Tokarev - Sept. 26, 2012, 2:56 p.m.
On 26.09.2012 17:46, Anthony Liguori wrote:
[]
> This is a good example of where we need improved documentation but I
> agree 100% with Peter.

So what do we do?

We've a clear bug, I can only fix it in the patch to the Debian
package, since I've been asked about this bug multiple times,
and I care about our users. It is at least consistent with the
expectations.  Maybe at the same time, it's a good idea to print
a warning about -nographic being deprecated, but this part should
definitely be done upstream first.

Thanks,

/mjt
Michael Tokarev - Oct. 27, 2012, 11:23 a.m.
On 26.09.2012 18:56, Michael Tokarev wrote:
> On 26.09.2012 17:46, Anthony Liguori wrote:
> []
>> This is a good example of where we need improved documentation but I
>> agree 100% with Peter.
> 
> So what do we do?
> 
> We've a clear bug, I can only fix it in the patch to the Debian
> package, since I've been asked about this bug multiple times,
> and I care about our users. It is at least consistent with the
> expectations.  Maybe at the same time, it's a good idea to print
> a warning about -nographic being deprecated, but this part should
> definitely be done upstream first.

Ping?

I still don't see why

 -nographic -daemonize

makes no sence while

 -curses -daemonize

does?  (Patch for the latter has been accepted right before my patch,
and even sent to -stable, but my patch is rejected without any conclusion).

Let's be at least consistent and either apply both or reject both, okay?

Thanks,

/mjt
Peter Maydell - Oct. 27, 2012, 11:33 a.m.
On 27 October 2012 12:23, Michael Tokarev <mjt@tls.msk.ru> wrote:
>
> I still don't see why
>
>  -nographic -daemonize
>
> makes no sence while
>
>  -curses -daemonize
>
> does?

My vote is that neither of these combinations makes sense.

-- PMM

Patch

diff --git a/vl.c b/vl.c
index 48049ef..a210ff9 100644
--- a/vl.c
+++ b/vl.c
@@ -3392,30 +3392,39 @@  int main(int argc, char **argv, char **envp)
         default_sdcard = 0;
     }
 
-    if (display_type == DT_NOGRAPHIC) {
-        if (default_parallel)
-            add_device_config(DEV_PARALLEL, "null");
+    /* Create default monitor, serial, parallel and virtcon devices. */
+    /* reuse optarg variable */
+    optarg = NULL;
+    if (display_type != DT_NOGRAPHIC) {
+        /* regular case, all devices directed to the guest console */
+        optarg = "vc:80Cx24C";
+    } else if (is_daemonized()) {
+        /* nographic and daemonize, everything => null */
+        optarg = "null";
+    } else {
+        /* nographic and no daemonize */
+        /* can't have both serial and virtcon on stdio */
         if (default_serial && default_monitor) {
             add_device_config(DEV_SERIAL, "mon:stdio");
         } else if (default_virtcon && default_monitor) {
             add_device_config(DEV_VIRTCON, "mon:stdio");
         } else {
-            if (default_serial)
-                add_device_config(DEV_SERIAL, "stdio");
-            if (default_virtcon)
-                add_device_config(DEV_VIRTCON, "stdio");
-            if (default_monitor)
-                monitor_parse("stdio", "readline");
+            optarg = "stdio";
         }
-    } else {
-        if (default_serial)
-            add_device_config(DEV_SERIAL, "vc:80Cx24C");
-        if (default_parallel)
-            add_device_config(DEV_PARALLEL, "vc:80Cx24C");
-        if (default_monitor)
-            monitor_parse("vc:80Cx24C", "readline");
-        if (default_virtcon)
-            add_device_config(DEV_VIRTCON, "vc:80Cx24C");
+    }
+    if (optarg && default_serial) {
+        add_device_config(DEV_SERIAL, optarg);
+    }
+    if (default_parallel) {
+        /* parallel port is connected console or to null */
+        add_device_config(DEV_PARALLEL,
+                          display_type == DT_NOGRAPHIC ? "null" : optarg);
+    }
+    if (optarg && default_monitor) {
+        monitor_parse(optarg, "readline");
+    }
+    if (optarg && default_virtcon) {
+        add_device_config(DEV_VIRTCON, optarg);
     }
 
     socket_init();