| Message ID | 1348584532-21914-1-git-send-email-mjt@msgid.tls.msk.ru |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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
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
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
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
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();
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(-)