Re: [Qemu-devel] [PATCH] display: stop using DT_NOGRAPHIC, use DT_NONE

[Anthony Liguori <aliguori@us.ibm.com>]

Michael Tokarev <mjt@tls.msk.ru> writes:

> It looks like initially there was -nographic option to turn
> off display, now there's another option of the same sort,
> -display none.  But code in other places of qemu checks for
> DT_NOGRAPHIC and does not work well with -display none.
> Make DT_NOGRAPHIC an internal version which selects DT_NONE,
> and check for that in all other places where previously we
> checked for DT_NOGRAPHIC.
>
> While at it, rename two private variants of display (DT_DEFAULT
> and DT_NOGRAPHIC) to use two underscores and make them negative,
> and set DT_NONE to 0.
>
> This should fix the issue of non-working sun serial console
> with the suggested replacement of -nographic which is
> -display none.
>
> I'm not still sure we really want to check for display type
> in qemu-char.c where we allow/disallow signals delivery from
> terminal, -- for other display types (CURSES) this makes no
> good sense.
>
> If the fix is correct, it is a stable material.

Breaks make check:

main-loop: WARNING: I/O thread spun for 1000 iterations
**
ERROR:/home/aliguori/git/qemu/tests/fw_cfg-test.c:63:test_fw_cfg_nographic: assertion failed (qfw_cfg_get_u16(fw_cfg, FW_CFG_NOGRAPHIC) == 0): (1 == 0)
GTester: last random seed: R02S25031265f05e4d41efcf758c9ef6043b

Regards,

Anthony Liguori

>
> Cc: Todd T. Fries <todd@fries.net>
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  hw/lm32/milkymist-hw.h  |    2 +-
>  hw/nvram/fw_cfg.c       |    2 +-
>  hw/sparc/sun4m.c        |    2 +-
>  include/sysemu/sysemu.h |    6 +++---
>  qemu-char.c             |    4 ++--
>  vl.c                    |   15 +++++++--------
>  6 files changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/hw/lm32/milkymist-hw.h b/hw/lm32/milkymist-hw.h
> index 5317ce6..59af720 100644
> --- a/hw/lm32/milkymist-hw.h
> +++ b/hw/lm32/milkymist-hw.h
> @@ -107,7 +107,7 @@ static inline DeviceState *milkymist_tmu2_create(hwaddr base,
>      int nelements;
>      int ver_major, ver_minor;
>  
> -    if (display_type == DT_NOGRAPHIC) {
> +    if (display_type == DT_NONE) {
>          return NULL;
>      }
>  
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 3c255ce..b3d163a 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -510,7 +510,7 @@ FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
>      }
>      fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4);
>      fw_cfg_add_bytes(s, FW_CFG_UUID, qemu_uuid, 16);
> -    fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)(display_type == DT_NOGRAPHIC));
> +    fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)(display_type == DT_NONE));
>      fw_cfg_add_i16(s, FW_CFG_NB_CPUS, (uint16_t)smp_cpus);
>      fw_cfg_add_i16(s, FW_CFG_BOOT_MENU, (uint16_t)boot_menu);
>      fw_cfg_bootsplash(s);
> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
> index 0e86ca7..c1d42ec 100644
> --- a/hw/sparc/sun4m.c
> +++ b/hw/sparc/sun4m.c
> @@ -919,7 +919,7 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef, ram_addr_t RAM_size,
>      slavio_timer_init_all(hwdef->counter_base, slavio_irq[19], slavio_cpu_irq, smp_cpus);
>  
>      slavio_serial_ms_kbd_init(hwdef->ms_kb_base, slavio_irq[14],
> -                              display_type == DT_NOGRAPHIC, ESCC_CLOCK, 1);
> +                              display_type == DT_NONE, ESCC_CLOCK, 1);
>      /* Slavio TTYA (base+4, Linux ttyS0) is the first QEMU serial device
>         Slavio TTYB (base+0, Linux ttyS1) is the second QEMU serial device */
>      escc_init(hwdef->serial_base, slavio_irq[15], slavio_irq[15],
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 2fb71af..fba7a7a 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -87,12 +87,12 @@ void do_info_slirp(Monitor *mon);
>  
>  typedef enum DisplayType
>  {
> -    DT_DEFAULT,
> +    DT__DEFAULT = -1,   /* private */
> +    DT__NOGRAPHIC = -2, /* private */
> +    DT_NONE = 0,
>      DT_CURSES,
>      DT_SDL,
>      DT_GTK,
> -    DT_NOGRAPHIC,
> -    DT_NONE,
>  } DisplayType;
>  
>  extern int autostart;
> diff --git a/qemu-char.c b/qemu-char.c
> index 2c3cfe6..3434971 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -955,7 +955,7 @@ static CharDriverState *qemu_chr_open_stdio(ChardevStdio *opts)
>      chr = qemu_chr_open_fd(0, 1);
>      chr->chr_close = qemu_chr_close_stdio;
>      chr->chr_set_echo = qemu_chr_set_echo_stdio;
> -    stdio_allow_signal = display_type != DT_NOGRAPHIC;
> +    stdio_allow_signal = display_type != DT_NONE;
>      if (opts->has_signal) {
>          stdio_allow_signal = opts->signal;
>      }
> @@ -3066,7 +3066,7 @@ static void qemu_chr_parse_stdio(QemuOpts *opts, ChardevBackend *backend,
>      backend->stdio = g_new0(ChardevStdio, 1);
>      backend->stdio->has_signal = true;
>      backend->stdio->signal =
> -        qemu_opt_get_bool(opts, "signal", display_type != DT_NOGRAPHIC);
> +        qemu_opt_get_bool(opts, "signal", display_type != DT_NONE);
>  }
>  
>  static void qemu_chr_parse_serial(QemuOpts *opts, ChardevBackend *backend,
> diff --git a/vl.c b/vl.c
> index f94ec9c..0269965 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -183,7 +183,7 @@ static const char *data_dir[16];
>  static int data_dir_idx;
>  const char *bios_name = NULL;
>  enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB;
> -DisplayType display_type = DT_DEFAULT;
> +DisplayType display_type = DT__DEFAULT;
>  static int display_remote;
>  const char* keyboard_layout = NULL;
>  ram_addr_t ram_size;
> @@ -2152,7 +2152,7 @@ static void select_vgahw (const char *p)
>  static DisplayType select_display(const char *p)
>  {
>      const char *opts;
> -    DisplayType display = DT_DEFAULT;
> +    DisplayType display = DT__DEFAULT;
>  
>      if (strstart(p, "sdl", &opts)) {
>  #ifdef CONFIG_SDL
> @@ -3093,7 +3093,7 @@ int main(int argc, char **argv, char **envp)
>                  display_type = select_display(optarg);
>                  break;
>              case QEMU_OPTION_nographic:
> -                display_type = DT_NOGRAPHIC;
> +                display_type = DT__NOGRAPHIC;
>                  break;
>              case QEMU_OPTION_curses:
>  #ifdef CONFIG_CURSES
> @@ -4018,7 +4018,7 @@ int main(int argc, char **argv, char **envp)
>           * -nographic _and_ redirects all ports explicitly - this is valid
>           * usage, -nographic is just a no-op in this case.
>           */
> -        if (display_type == DT_NOGRAPHIC
> +        if (display_type == DT__NOGRAPHIC
>              && (default_parallel || default_serial
>                  || default_monitor || default_virtcon)) {
>              fprintf(stderr, "-nographic can not be used with -daemonize\n");
> @@ -4032,7 +4032,8 @@ int main(int argc, char **argv, char **envp)
>  #endif
>      }
>  
> -    if (display_type == DT_NOGRAPHIC) {
> +    if (display_type == DT__NOGRAPHIC) {
> +        display_type = DT_NONE;
>          if (default_parallel)
>              add_device_config(DEV_PARALLEL, "null");
>          if (default_serial && default_monitor) {
> @@ -4066,7 +4067,7 @@ int main(int argc, char **argv, char **envp)
>          }
>      }
>  
> -    if (display_type == DT_DEFAULT && !display_remote) {
> +    if (display_type == DT__DEFAULT && !display_remote) {
>  #if defined(CONFIG_GTK)
>          display_type = DT_GTK;
>  #elif defined(CONFIG_SDL) || defined(CONFIG_COCOA)
> @@ -4333,8 +4334,6 @@ int main(int argc, char **argv, char **envp)
>  
>      /* init local displays */
>      switch (display_type) {
> -    case DT_NOGRAPHIC:
> -        break;
>  #if defined(CONFIG_CURSES)
>      case DT_CURSES:
>          curses_display_init(ds, full_screen);
> -- 
> 1.7.10.4