[Qemu-devel] [PATCH v2] stop using stdio for monitor/serial/etc with -daemonize

[Qemu-devel] [PATCH v2] stop using stdio for monitor/serial/etc with -daemonize

[Michael Tokarev]

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

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

Re: [Qemu-devel] [PATCH v2] stop using stdio for monitor/serial/etc with -daemonize

[Anthony Liguori]

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

Re: [Qemu-devel] [PATCH v2] stop using stdio for monitor/serial/etc with -daemonize

[Michael Tokarev]

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

Re: [Qemu-devel] [PATCH v2] stop using stdio for monitor/serial/etc with -daemonize

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH v2] stop using stdio for monitor/serial/etc with -daemonize

[Michael Tokarev]

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

Re: [Qemu-devel] [PATCH v2] stop using stdio for monitor/serial/etc with -daemonize

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH v2] stop using stdio for monitor/serial/etc with -daemonize

[Anthony Liguori]

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

Re: [Qemu-devel] [PATCH v2] stop using stdio for monitor/serial/etc with -daemonize

[Michael Tokarev]

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

Re: [Qemu-devel] [PATCH v2] stop using stdio for monitor/serial/etc with -daemonize

[Michael Tokarev]

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

Re: [Qemu-devel] [PATCH v2] stop using stdio for monitor/serial/etc with -daemonize

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH v2] stop using stdio for monitor/serial/etc with -daemonize

[Michael Tokarev]

[-- Attachment #1: Type: text/plain, Size: 1625 bytes --]

On 27.10.2012 15:33, Peter Maydell wrote:
> 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.

I agree.  Well, almost -- to me, -curses -daemonize makes much
less sence than -nographic -daemonize - at least when you think
about it, ie, when you rely on common sense.  When you look at
the docs, it becomes apparent that -nographic does something
else when you thought it is, and so both becomes equally
non-sentical.

Actually I wanted to error out on -nographic -daemonize (it is "my"
bug), but after seeing 995ee2bf469de6bbe5ce133ec853392b2a4ce34c
(which is the "fix" for -curses -daemonize), I decided to fix it
as well.

So, maybe we should fix both by disallowing both combinations?
Like the attached patch does?

I'd rather have -nographic work with -daemonize, since the
alternative - shown in the patch comment - is rather long and
it is easy to forget to "nullify" some option, while -nographic
can do that easy and it is convinient, but if people dislikes
such natural and easy-for-the-user solutions, I wont insist.

Note that the actual outcome of both is the same -- after using
either of the two combination (without the above-mentioned fix),
the terminal switches to raw mode and little can be done with
it.


It is a real PITA that these rather trivial things require so much
discussing and stays known but unfixed for so long, and much more
important things gets less time and energy as the result.


/mjt

[-- Attachment #2: 0001-disallow-daemonize-with-curses-display-or-nographic.patch --]
[-- Type: text/x-patch, Size: 2739 bytes --]

>From 09808040ef70f62f0ffefae3a95e0d0fc7ef09a5 Mon Sep 17 00:00:00 2001
From: Michael Tokarev <mjt@tls.msk.ru>
Date: Sat, 27 Oct 2012 16:03:34 +0400
Subject: [PATCH] disallow -daemonize with curses display or -nographic

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.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 vl.c |   24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

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 v2] stop using stdio for monitor/serial/etc with -daemonize

[Blue Swirl]

On Sat, Oct 27, 2012 at 12:15 PM, Michael Tokarev <mjt@tls.msk.ru> wrote:
> On 27.10.2012 15:33, Peter Maydell wrote:
>> 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.
>
> I agree.  Well, almost -- to me, -curses -daemonize makes much
> less sence than -nographic -daemonize - at least when you think
> about it, ie, when you rely on common sense.  When you look at
> the docs, it becomes apparent that -nographic does something
> else when you thought it is, and so both becomes equally
> non-sentical.
>
> Actually I wanted to error out on -nographic -daemonize (it is "my"
> bug), but after seeing 995ee2bf469de6bbe5ce133ec853392b2a4ce34c
> (which is the "fix" for -curses -daemonize), I decided to fix it
> as well.
>
> So, maybe we should fix both by disallowing both combinations?
> Like the attached patch does?
>
> I'd rather have -nographic work with -daemonize, since the
> alternative - shown in the patch comment - is rather long and
> it is easy to forget to "nullify" some option, while -nographic
> can do that easy and it is convinient, but if people dislikes
> such natural and easy-for-the-user solutions, I wont insist.

Instead of checking just for -nographic or -curses, can we forbid use
of any stdio chardev?

>
> Note that the actual outcome of both is the same -- after using
> either of the two combination (without the above-mentioned fix),
> the terminal switches to raw mode and little can be done with
> it.
>
>
> It is a real PITA that these rather trivial things require so much
> discussing and stays known but unfixed for so long, and much more
> important things gets less time and energy as the result.

Perfect is the enemy of good. It's also too easy to break things since
the design features are not described and tested comprehensively.

>
>
> /mjt

Re: [Qemu-devel] [PATCH v2] stop using stdio for monitor/serial/etc with -daemonize

[Michael Tokarev]

On 27.10.2012 16:48, Blue Swirl wrote:
[]
>> I'd rather have -nographic work with -daemonize, since the
>> alternative - shown in the patch comment - is rather long and
>> it is easy to forget to "nullify" some option, while -nographic
>> can do that easy and it is convinient, but if people dislikes
>> such natural and easy-for-the-user solutions, I wont insist.
> 
> Instead of checking just for -nographic or -curses, can we forbid use
> of any stdio chardev?

I think that'll be quite a bit more difficult.  Sure, say,

  -serial stdio -daemonize

now has the same problem as original

  -nographic -daemonize.

It is just now after you mentioned it I realized this omission.
And it is exactly the same thing actually - we initialize
stdio for the serial port, in both cases, and it switches
the tty to raw mode.

So this patch is insufficient indeed, we still have the
same issue, and once -nographic -daemonize is disallowed,
we've much better chances to hit this issue using -serial.
Oh well.

Hmm.  Maybe init stdio chardev for something "else" in case
of -nographic?

[]
>> It is a real PITA that these rather trivial things require so much
>> discussing and stays known but unfixed for so long, and much more
>> important things gets less time and energy as the result.
> 
> Perfect is the enemy of good. It's also too easy to break things since
> the design features are not described and tested comprehensively.

Well, bugs aren't perfect or good, they're bad.  And any breakage
can be fixed once detected, it isn't like we've some very deep
dependencies with very distant and hidden effects -- I'm talking
about rather trivial things really.

Thanks,

/mjt

Re: [Qemu-devel] [PATCH v2] stop using stdio for monitor/serial/etc with -daemonize

[Michael Tokarev]

On 27.10.2012 16:55, Michael Tokarev wrote:
> On 27.10.2012 16:48, Blue Swirl wrote:
> []
>>> I'd rather have -nographic work with -daemonize, since the
>>> alternative - shown in the patch comment - is rather long and
>>> it is easy to forget to "nullify" some option, while -nographic
>>> can do that easy and it is convinient, but if people dislikes
>>> such natural and easy-for-the-user solutions, I wont insist.
>>
>> Instead of checking just for -nographic or -curses, can we forbid use
>> of any stdio chardev?
> 
> I think that'll be quite a bit more difficult.  Sure, say,
> 
>   -serial stdio -daemonize
> 
> now has the same problem as original
> 
>   -nographic -daemonize.
> 
> It is just now after you mentioned it I realized this omission.
> And it is exactly the same thing actually - we initialize
> stdio for the serial port, in both cases, and it switches
> the tty to raw mode.
> 
> So this patch is insufficient indeed, we still have the
> same issue, and once -nographic -daemonize is disallowed,
> we've much better chances to hit this issue using -serial.
> Oh well.
> 
> Hmm.  Maybe init stdio chardev for something "else" in case
> of -nographic?

This, together with my previous patch, appears to work fine:

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

(there's no need to add this to windows version, since
we don't daemonize on windows).

Thanks,

/mjt