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

login
register
mail settings
Submitter Michael Tokarev
Date Oct. 27, 2012, 12:15 p.m.
Message ID <508BD064.9080505@msgid.tls.msk.ru>
Download mbox | patch
Permalink /patch/194613/
State New
Headers show

Comments

Michael Tokarev - Oct. 27, 2012, 12:15 p.m.
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
Blue Swirl - Oct. 27, 2012, 12:48 p.m.
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
Michael Tokarev - Oct. 27, 2012, 12:55 p.m.
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

Patch

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