[v2] ui/vnc.c: polish vnc_init_func

Message ID 20181012115013.29555-1-fli@suse.com
State New
Headers show
Series
  • [v2] ui/vnc.c: polish vnc_init_func
Related show

Commit Message

Fei Li Oct. 12, 2018, 11:50 a.m.
Add a new Error parameter for vnc_display_init() to handle errors
in its caller: vnc_init_func(), just like vnc_display_open() does.
And let its callees propagate the Error, like init_keyboard_layout().

Signed-off-by: Fei Li <fli@suse.com>
---
 include/ui/console.h |  2 +-
 ui/curses.c          |  4 +++-
 ui/keymaps.c         | 11 ++++++-----
 ui/keymaps.h         |  2 +-
 ui/sdl.c             |  4 +++-
 ui/vnc.c             | 16 +++++++++++-----
 6 files changed, 25 insertions(+), 14 deletions(-)

Comments

Fei Li Oct. 12, 2018, 11:59 a.m. | #1
On 10/12/2018 07:50 PM, Fei Li wrote:
> Add a new Error parameter for vnc_display_init() to handle errors
> in its caller: vnc_init_func(), just like vnc_display_open() does.
> And let its callees propagate the Error, like init_keyboard_layout().
>
> Signed-off-by: Fei Li <fli@suse.com>
As this version do a big adjustment, remove the former Reviewed-By
for the time being.
> ---
>   include/ui/console.h |  2 +-
>   ui/curses.c          |  4 +++-
>   ui/keymaps.c         | 11 ++++++-----
>   ui/keymaps.h         |  2 +-
>   ui/sdl.c             |  4 +++-
>   ui/vnc.c             | 16 +++++++++++-----
>   6 files changed, 25 insertions(+), 14 deletions(-)
>
> diff --git a/include/ui/console.h b/include/ui/console.h
> index fb969caf70..c17803c530 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -453,7 +453,7 @@ void qemu_display_early_init(DisplayOptions *opts);
>   void qemu_display_init(DisplayState *ds, DisplayOptions *opts);
>   
>   /* vnc.c */
> -void vnc_display_init(const char *id);
> +void vnc_display_init(const char *id, Error **errp);
>   void vnc_display_open(const char *id, Error **errp);
>   void vnc_display_add_client(const char *id, int csock, bool skipauth);
>   int vnc_display_password(const char *id, const char *password);
> diff --git a/ui/curses.c b/ui/curses.c
> index 59d819fd4d..60c0962334 100644
> --- a/ui/curses.c
> +++ b/ui/curses.c
> @@ -28,6 +28,7 @@
>   #include <termios.h>
>   #endif
>   
> +#include "qapi/error.h"
>   #include "qemu-common.h"
>   #include "ui/console.h"
>   #include "ui/input.h"
> @@ -421,7 +422,8 @@ static void curses_keyboard_setup(void)
>           keyboard_layout = "en-us";
>   #endif
>       if(keyboard_layout) {
> -        kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout);
> +        kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout,
> +                                          &error_fatal);
>           if (!kbd_layout)
>               exit(1);
>       }
> diff --git a/ui/keymaps.c b/ui/keymaps.c
> index 43fe604724..52d432e856 100644
> --- a/ui/keymaps.c
> +++ b/ui/keymaps.c
> @@ -27,6 +27,7 @@
>   #include "sysemu/sysemu.h"
>   #include "trace.h"
>   #include "qemu/error-report.h"
> +#include "qapi/error.h"
>   
>   struct keysym2code {
>       uint32_t count;
> @@ -81,7 +82,7 @@ static void add_keysym(char *line, int keysym, int keycode, kbd_layout_t *k)
>   
>   static kbd_layout_t *parse_keyboard_layout(const name2keysym_t *table,
>                                              const char *language,
> -                                           kbd_layout_t *k)
> +                                           kbd_layout_t *k, Error **errp)
>   {
>       FILE *f;
>       char * filename;
> @@ -94,7 +95,7 @@ static kbd_layout_t *parse_keyboard_layout(const name2keysym_t *table,
>       f = filename ? fopen(filename, "r") : NULL;
>       g_free(filename);
>       if (!f) {
> -        fprintf(stderr, "Could not read keymap file: '%s'\n", language);
> +        error_setg(errp, "could not read keymap file: '%s'", language);
>           return NULL;
>       }
>   
> @@ -118,7 +119,7 @@ static kbd_layout_t *parse_keyboard_layout(const name2keysym_t *table,
>               continue;
>           }
>           if (!strncmp(line, "include ", 8)) {
> -            parse_keyboard_layout(table, line + 8, k);
> +            parse_keyboard_layout(table, line + 8, k, errp);
>           } else {
>               int offset = 0;
>               while (line[offset] != 0 &&
> @@ -170,9 +171,9 @@ static kbd_layout_t *parse_keyboard_layout(const name2keysym_t *table,
>   
>   
>   kbd_layout_t *init_keyboard_layout(const name2keysym_t *table,
> -                                   const char *language)
> +                                   const char *language, Error **errp)
>   {
> -    return parse_keyboard_layout(table, language, NULL);
> +    return parse_keyboard_layout(table, language, NULL, errp);
>   }
>   
>   
> diff --git a/ui/keymaps.h b/ui/keymaps.h
> index 0693588225..98213a4191 100644
> --- a/ui/keymaps.h
> +++ b/ui/keymaps.h
> @@ -53,7 +53,7 @@ typedef struct {
>   typedef struct kbd_layout_t kbd_layout_t;
>   
>   kbd_layout_t *init_keyboard_layout(const name2keysym_t *table,
> -                                   const char *language);
> +                                   const char *language, Error **errp);
>   int keysym2scancode(kbd_layout_t *k, int keysym,
>                       bool shift, bool altgr, bool ctrl);
>   int keycode_is_keypad(kbd_layout_t *k, int keycode);
> diff --git a/ui/sdl.c b/ui/sdl.c
> index a5fd503c25..b33df70f4d 100644
> --- a/ui/sdl.c
> +++ b/ui/sdl.c
> @@ -29,6 +29,7 @@
>   #include <SDL.h>
>   #include <SDL_syswm.h>
>   
> +#include "qapi/error.h"
>   #include "qemu-common.h"
>   #include "qemu/cutils.h"
>   #include "ui/console.h"
> @@ -917,7 +918,8 @@ static void sdl1_display_init(DisplayState *ds, DisplayOptions *o)
>           keyboard_layout = "en-us";
>   #endif
>       if(keyboard_layout) {
> -        kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout);
> +        kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout,
> +                                          &error_fatal);
>           if (!kbd_layout)
>               exit(1);
>       }
> diff --git a/ui/vnc.c b/ui/vnc.c
> index cf221c83cc..03c70b16f6 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -3205,7 +3205,7 @@ static const DisplayChangeListenerOps dcl_ops = {
>       .dpy_cursor_define    = vnc_dpy_cursor_define,
>   };
>   
> -void vnc_display_init(const char *id)
> +void vnc_display_init(const char *id, Error **errp)
>   {
>       VncDisplay *vd;
>   
> @@ -3222,13 +3222,14 @@ void vnc_display_init(const char *id)
>   
>       if (keyboard_layout) {
>           trace_vnc_key_map_init(keyboard_layout);
> -        vd->kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout);
> +        vd->kbd_layout = init_keyboard_layout(name2keysym,
> +                                              keyboard_layout, errp);
>       } else {
> -        vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us");
> +        vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us", errp);
>       }
>   
>       if (!vd->kbd_layout) {
> -        exit(1);
> +        return;
>       }
>   
>       vd->share_policy = VNC_SHARE_POLICY_ALLOW_EXCLUSIVE;
> @@ -4079,7 +4080,12 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
>       char *id = (char *)qemu_opts_id(opts);
>   
>       assert(id);
> -    vnc_display_init(id);
> +    vnc_display_init(id, &local_err);
> +    if (local_err) {
> +        error_prepend(&local_err, "Failed to init VNC server: ");
> +        error_propagate(errp, local_err);
> +        return -1;
> +    }
>       vnc_display_open(id, &local_err);
>       if (local_err != NULL) {
>           error_reportf_err(local_err, "Failed to start VNC server: ");
Philippe Mathieu-Daudé Oct. 12, 2018, 12:42 p.m. | #2
Hi Fei,

On 12/10/2018 13:50, Fei Li wrote:
> Add a new Error parameter for vnc_display_init() to handle errors
> in its caller: vnc_init_func(), just like vnc_display_open() does.
> And let its callees propagate the Error, like init_keyboard_layout().
> 
> Signed-off-by: Fei Li <fli@suse.com>
> ---
>  include/ui/console.h |  2 +-
>  ui/curses.c          |  4 +++-
>  ui/keymaps.c         | 11 ++++++-----
>  ui/keymaps.h         |  2 +-
>  ui/sdl.c             |  4 +++-
>  ui/vnc.c             | 16 +++++++++++-----
>  6 files changed, 25 insertions(+), 14 deletions(-)
> 
> diff --git a/include/ui/console.h b/include/ui/console.h
> index fb969caf70..c17803c530 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -453,7 +453,7 @@ void qemu_display_early_init(DisplayOptions *opts);
>  void qemu_display_init(DisplayState *ds, DisplayOptions *opts);
>  
>  /* vnc.c */
> -void vnc_display_init(const char *id);
> +void vnc_display_init(const char *id, Error **errp);
>  void vnc_display_open(const char *id, Error **errp);
>  void vnc_display_add_client(const char *id, int csock, bool skipauth);
>  int vnc_display_password(const char *id, const char *password);
> diff --git a/ui/curses.c b/ui/curses.c
> index 59d819fd4d..60c0962334 100644
> --- a/ui/curses.c
> +++ b/ui/curses.c
> @@ -28,6 +28,7 @@
>  #include <termios.h>
>  #endif
>  
> +#include "qapi/error.h"
>  #include "qemu-common.h"
>  #include "ui/console.h"
>  #include "ui/input.h"
> @@ -421,7 +422,8 @@ static void curses_keyboard_setup(void)
>          keyboard_layout = "en-us";
>  #endif
>      if(keyboard_layout) {
> -        kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout);
> +        kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout,
> +                                          &error_fatal);
>          if (!kbd_layout)
>              exit(1);

So you can now remove those 2 lines.

>      }
> diff --git a/ui/keymaps.c b/ui/keymaps.c
> index 43fe604724..52d432e856 100644
> --- a/ui/keymaps.c
> +++ b/ui/keymaps.c
> @@ -27,6 +27,7 @@
>  #include "sysemu/sysemu.h"
>  #include "trace.h"
>  #include "qemu/error-report.h"
> +#include "qapi/error.h"
>  
>  struct keysym2code {
>      uint32_t count;
> @@ -81,7 +82,7 @@ static void add_keysym(char *line, int keysym, int keycode, kbd_layout_t *k)
>  
>  static kbd_layout_t *parse_keyboard_layout(const name2keysym_t *table,
>                                             const char *language,
> -                                           kbd_layout_t *k)
> +                                           kbd_layout_t *k, Error **errp)
>  {
>      FILE *f;
>      char * filename;
> @@ -94,7 +95,7 @@ static kbd_layout_t *parse_keyboard_layout(const name2keysym_t *table,
>      f = filename ? fopen(filename, "r") : NULL;
>      g_free(filename);
>      if (!f) {
> -        fprintf(stderr, "Could not read keymap file: '%s'\n", language);
> +        error_setg(errp, "could not read keymap file: '%s'", language);
>          return NULL;
>      }
>  
> @@ -118,7 +119,7 @@ static kbd_layout_t *parse_keyboard_layout(const name2keysym_t *table,
>              continue;
>          }
>          if (!strncmp(line, "include ", 8)) {
> -            parse_keyboard_layout(table, line + 8, k);
> +            parse_keyboard_layout(table, line + 8, k, errp);
>          } else {
>              int offset = 0;
>              while (line[offset] != 0 &&
> @@ -170,9 +171,9 @@ static kbd_layout_t *parse_keyboard_layout(const name2keysym_t *table,
>  
>  
>  kbd_layout_t *init_keyboard_layout(const name2keysym_t *table,
> -                                   const char *language)
> +                                   const char *language, Error **errp)
>  {
> -    return parse_keyboard_layout(table, language, NULL);
> +    return parse_keyboard_layout(table, language, NULL, errp);
>  }
>  
>  
> diff --git a/ui/keymaps.h b/ui/keymaps.h
> index 0693588225..98213a4191 100644
> --- a/ui/keymaps.h
> +++ b/ui/keymaps.h
> @@ -53,7 +53,7 @@ typedef struct {
>  typedef struct kbd_layout_t kbd_layout_t;
>  
>  kbd_layout_t *init_keyboard_layout(const name2keysym_t *table,
> -                                   const char *language);
> +                                   const char *language, Error **errp);
>  int keysym2scancode(kbd_layout_t *k, int keysym,
>                      bool shift, bool altgr, bool ctrl);
>  int keycode_is_keypad(kbd_layout_t *k, int keycode);
> diff --git a/ui/sdl.c b/ui/sdl.c
> index a5fd503c25..b33df70f4d 100644
> --- a/ui/sdl.c
> +++ b/ui/sdl.c
> @@ -29,6 +29,7 @@
>  #include <SDL.h>
>  #include <SDL_syswm.h>
>  
> +#include "qapi/error.h"
>  #include "qemu-common.h"
>  #include "qemu/cutils.h"
>  #include "ui/console.h"
> @@ -917,7 +918,8 @@ static void sdl1_display_init(DisplayState *ds, DisplayOptions *o)
>          keyboard_layout = "en-us";
>  #endif
>      if(keyboard_layout) {
> -        kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout);
> +        kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout,
> +                                          &error_fatal);
>          if (!kbd_layout)
>              exit(1);
>      }
> diff --git a/ui/vnc.c b/ui/vnc.c
> index cf221c83cc..03c70b16f6 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -3205,7 +3205,7 @@ static const DisplayChangeListenerOps dcl_ops = {
>      .dpy_cursor_define    = vnc_dpy_cursor_define,
>  };
>  
> -void vnc_display_init(const char *id)
> +void vnc_display_init(const char *id, Error **errp)
>  {
>      VncDisplay *vd;
>  
> @@ -3222,13 +3222,14 @@ void vnc_display_init(const char *id)
>  
>      if (keyboard_layout) {
>          trace_vnc_key_map_init(keyboard_layout);
> -        vd->kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout);
> +        vd->kbd_layout = init_keyboard_layout(name2keysym,
> +                                              keyboard_layout, errp);
>      } else {
> -        vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us");
> +        vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us", errp);
>      }
>  
>      if (!vd->kbd_layout) {
> -        exit(1);
> +        return;
>      }
>  
>      vd->share_policy = VNC_SHARE_POLICY_ALLOW_EXCLUSIVE;
> @@ -4079,7 +4080,12 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
>      char *id = (char *)qemu_opts_id(opts);
>  
>      assert(id);
> -    vnc_display_init(id);
> +    vnc_display_init(id, &local_err);
> +    if (local_err) {
> +        error_prepend(&local_err, "Failed to init VNC server: ");
> +        error_propagate(errp, local_err);
> +        return -1;
> +    }
>      vnc_display_open(id, &local_err);
>      if (local_err != NULL) {
>          error_reportf_err(local_err, "Failed to start VNC server: ");
>
Markus Armbruster Oct. 15, 2018, 7:58 a.m. | #3
Fei Li <fli@suse.com> writes:

> Add a new Error parameter for vnc_display_init() to handle errors
> in its caller: vnc_init_func(), just like vnc_display_open() does.
> And let its callees propagate the Error, like init_keyboard_layout().
>
> Signed-off-by: Fei Li <fli@suse.com>
> ---
>  include/ui/console.h |  2 +-
>  ui/curses.c          |  4 +++-
>  ui/keymaps.c         | 11 ++++++-----
>  ui/keymaps.h         |  2 +-
>  ui/sdl.c             |  4 +++-
>  ui/vnc.c             | 16 +++++++++++-----
>  6 files changed, 25 insertions(+), 14 deletions(-)
>
> diff --git a/include/ui/console.h b/include/ui/console.h
> index fb969caf70..c17803c530 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -453,7 +453,7 @@ void qemu_display_early_init(DisplayOptions *opts);
>  void qemu_display_init(DisplayState *ds, DisplayOptions *opts);
>  
>  /* vnc.c */
> -void vnc_display_init(const char *id);
> +void vnc_display_init(const char *id, Error **errp);
>  void vnc_display_open(const char *id, Error **errp);
>  void vnc_display_add_client(const char *id, int csock, bool skipauth);
>  int vnc_display_password(const char *id, const char *password);
> diff --git a/ui/curses.c b/ui/curses.c
> index 59d819fd4d..60c0962334 100644
> --- a/ui/curses.c
> +++ b/ui/curses.c
> @@ -28,6 +28,7 @@
>  #include <termios.h>
>  #endif
>  
> +#include "qapi/error.h"
>  #include "qemu-common.h"
>  #include "ui/console.h"
>  #include "ui/input.h"
> @@ -421,7 +422,8 @@ static void curses_keyboard_setup(void)
>          keyboard_layout = "en-us";
>  #endif
>      if(keyboard_layout) {
> -        kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout);
> +        kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout,
> +                                          &error_fatal);
>          if (!kbd_layout)
>              exit(1);

As Philippe pointed out, this exit() is now unreachable.

>      }
> diff --git a/ui/keymaps.c b/ui/keymaps.c
> index 43fe604724..52d432e856 100644
> --- a/ui/keymaps.c
> +++ b/ui/keymaps.c
> @@ -27,6 +27,7 @@
>  #include "sysemu/sysemu.h"
>  #include "trace.h"
>  #include "qemu/error-report.h"
> +#include "qapi/error.h"
>  
>  struct keysym2code {
>      uint32_t count;
> @@ -81,7 +82,7 @@ static void add_keysym(char *line, int keysym, int keycode, kbd_layout_t *k)
>  
>  static kbd_layout_t *parse_keyboard_layout(const name2keysym_t *table,
>                                             const char *language,
`> -                                           kbd_layout_t *k)
> +                                           kbd_layout_t *k, Error **errp)
>  {
>      FILE *f;
>      char * filename;
> @@ -94,7 +95,7 @@ static kbd_layout_t *parse_keyboard_layout(const name2keysym_t *table,
>      f = filename ? fopen(filename, "r") : NULL;
>      g_free(filename);
>      if (!f) {
> -        fprintf(stderr, "Could not read keymap file: '%s'\n", language);
> +        error_setg(errp, "could not read keymap file: '%s'", language);
>          return NULL;
>      }
>  
> @@ -118,7 +119,7 @@ static kbd_layout_t *parse_keyboard_layout(const name2keysym_t *table,
>              continue;
>          }
>          if (!strncmp(line, "include ", 8)) {
> -            parse_keyboard_layout(table, line + 8, k);
> +            parse_keyboard_layout(table, line + 8, k, errp);

Lacks error checking.

Before this patch, an error in an included file is reported to stderr,
but is otherwise ignored:

    $ cat kbd-layout
    include bad
    include worse
    $ qemu-system-x86_64 -nodefaults -S -monitor stdio -display vnc=:0 -k kbd-layout
    QEMU 3.0.50 monitor - type 'help' for more information
    (qemu) Could not read keymap file: 'bad'
    Could not read keymap file: 'worse'

Afterwards, the error gets stored in errp.  If a second include fails,
we attempt to store another one, which fails the error_setv()'s
assertion:

    $ qemu-system-x86_64 -nodefaults -S -display vnc=:0 -k kbd-layout
    qemu-system-x86_64: /work/armbru/qemu/util/error.c:57: error_setv: Assertion `*errp == NULL' failed.
    Aborted (core dumped)

If it's the only failed include, parse_keyboard_layout() sets an error
and returns succes, which is a no-no.  Callers using the return value to
determine success are prone to leak the error, or run into
error_setv()'s assertion.  Callers using the error are prone to leak the
returned kbd_layout_t.

Let's address this in a separate patch before this one.

               if (!parse_keyboard_layout(table, line + 8, k, errp))

>          } else {
>              int offset = 0;
>              while (line[offset] != 0 &&
> @@ -170,9 +171,9 @@ static kbd_layout_t *parse_keyboard_layout(const name2keysym_t *table,
>  
>  
>  kbd_layout_t *init_keyboard_layout(const name2keysym_t *table,
> -                                   const char *language)
> +                                   const char *language, Error **errp)
>  {
> -    return parse_keyboard_layout(table, language, NULL);
> +    return parse_keyboard_layout(table, language, NULL, errp);
>  }
>  
>  
> diff --git a/ui/keymaps.h b/ui/keymaps.h
> index 0693588225..98213a4191 100644
> --- a/ui/keymaps.h
> +++ b/ui/keymaps.h
> @@ -53,7 +53,7 @@ typedef struct {
>  typedef struct kbd_layout_t kbd_layout_t;
>  
>  kbd_layout_t *init_keyboard_layout(const name2keysym_t *table,
> -                                   const char *language);
> +                                   const char *language, Error **errp);
>  int keysym2scancode(kbd_layout_t *k, int keysym,
>                      bool shift, bool altgr, bool ctrl);
>  int keycode_is_keypad(kbd_layout_t *k, int keycode);
> diff --git a/ui/sdl.c b/ui/sdl.c
> index a5fd503c25..b33df70f4d 100644
> --- a/ui/sdl.c
> +++ b/ui/sdl.c
> @@ -29,6 +29,7 @@
>  #include <SDL.h>
>  #include <SDL_syswm.h>
>  
> +#include "qapi/error.h"
>  #include "qemu-common.h"
>  #include "qemu/cutils.h"
>  #include "ui/console.h"
> @@ -917,7 +918,8 @@ static void sdl1_display_init(DisplayState *ds, DisplayOptions *o)
>          keyboard_layout = "en-us";
>  #endif
>      if(keyboard_layout) {
> -        kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout);
> +        kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout,
> +                                          &error_fatal);
>          if (!kbd_layout)
>              exit(1);

Again, exit() is now unreachable.

>      }
> diff --git a/ui/vnc.c b/ui/vnc.c
> index cf221c83cc..03c70b16f6 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -3205,7 +3205,7 @@ static const DisplayChangeListenerOps dcl_ops = {
>      .dpy_cursor_define    = vnc_dpy_cursor_define,
>  };
>  
> -void vnc_display_init(const char *id)
> +void vnc_display_init(const char *id, Error **errp)
>  {
>      VncDisplay *vd;
>  
> @@ -3222,13 +3222,14 @@ void vnc_display_init(const char *id)
>  
>      if (keyboard_layout) {
>          trace_vnc_key_map_init(keyboard_layout);
> -        vd->kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout);
> +        vd->kbd_layout = init_keyboard_layout(name2keysym,
> +                                              keyboard_layout, errp);
>      } else {
> -        vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us");
> +        vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us", errp);
>      }
>  
>      if (!vd->kbd_layout) {
> -        exit(1);
> +        return;
>      }
>  
>      vd->share_policy = VNC_SHARE_POLICY_ALLOW_EXCLUSIVE;
> @@ -4079,7 +4080,12 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
>      char *id = (char *)qemu_opts_id(opts);
>  
>      assert(id);
> -    vnc_display_init(id);
> +    vnc_display_init(id, &local_err);
> +    if (local_err) {
> +        error_prepend(&local_err, "Failed to init VNC server: ");
> +        error_propagate(errp, local_err);
> +        return -1;
> +    }

Works, but is inconsistent with the handling of vnc_display_open()
errors, visible below.  Let's match that, and convert both to
error_propagate() in a followup patch.

>      vnc_display_open(id, &local_err);
>      if (local_err != NULL) {
>          error_reportf_err(local_err, "Failed to start VNC server: ");

I think the easiest way forward is for me to pick a touched up version
of this patch into a v2 of my "Replace some unwise uses of
error_report() & friends" series.  You'll receive a copy, of course.

Thanks!
Fei Li Oct. 15, 2018, 10:08 a.m. | #4
On 10/15/2018 03:58 PM, Markus Armbruster wrote:
> Fei Li <fli@suse.com> writes:
>
>> Add a new Error parameter for vnc_display_init() to handle errors
>> in its caller: vnc_init_func(), just like vnc_display_open() does.
>> And let its callees propagate the Error, like init_keyboard_layout().
>>
>> Signed-off-by: Fei Li <fli@suse.com>
>> ---
>>   include/ui/console.h |  2 +-
>>   ui/curses.c          |  4 +++-
>>   ui/keymaps.c         | 11 ++++++-----
>>   ui/keymaps.h         |  2 +-
>>   ui/sdl.c             |  4 +++-
>>   ui/vnc.c             | 16 +++++++++++-----
>>   6 files changed, 25 insertions(+), 14 deletions(-)
>>
>> diff --git a/include/ui/console.h b/include/ui/console.h
>> index fb969caf70..c17803c530 100644
>> --- a/include/ui/console.h
>> +++ b/include/ui/console.h
>> @@ -453,7 +453,7 @@ void qemu_display_early_init(DisplayOptions *opts);
>>   void qemu_display_init(DisplayState *ds, DisplayOptions *opts);
>>   
>>   /* vnc.c */
>> -void vnc_display_init(const char *id);
>> +void vnc_display_init(const char *id, Error **errp);
>>   void vnc_display_open(const char *id, Error **errp);
>>   void vnc_display_add_client(const char *id, int csock, bool skipauth);
>>   int vnc_display_password(const char *id, const char *password);
>> diff --git a/ui/curses.c b/ui/curses.c
>> index 59d819fd4d..60c0962334 100644
>> --- a/ui/curses.c
>> +++ b/ui/curses.c
>> @@ -28,6 +28,7 @@
>>   #include <termios.h>
>>   #endif
>>   
>> +#include "qapi/error.h"
>>   #include "qemu-common.h"
>>   #include "ui/console.h"
>>   #include "ui/input.h"
>> @@ -421,7 +422,8 @@ static void curses_keyboard_setup(void)
>>           keyboard_layout = "en-us";
>>   #endif
>>       if(keyboard_layout) {
>> -        kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout);
>> +        kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout,
>> +                                          &error_fatal);
>>           if (!kbd_layout)
>>               exit(1);
> As Philippe pointed out, this exit() is now unreachable.
Right, should be deleted along with the below one.
>
>>       }
>> diff --git a/ui/keymaps.c b/ui/keymaps.c
>> index 43fe604724..52d432e856 100644
>> --- a/ui/keymaps.c
>> +++ b/ui/keymaps.c
>> @@ -27,6 +27,7 @@
>>   #include "sysemu/sysemu.h"
>>   #include "trace.h"
>>   #include "qemu/error-report.h"
>> +#include "qapi/error.h"
>>   
>>   struct keysym2code {
>>       uint32_t count;
>> @@ -81,7 +82,7 @@ static void add_keysym(char *line, int keysym, int keycode, kbd_layout_t *k)
>>   
>>   static kbd_layout_t *parse_keyboard_layout(const name2keysym_t *table,
>>                                              const char *language,
> `> -                                           kbd_layout_t *k)
>> +                                           kbd_layout_t *k, Error **errp)
>>   {
>>       FILE *f;
>>       char * filename;
>> @@ -94,7 +95,7 @@ static kbd_layout_t *parse_keyboard_layout(const name2keysym_t *table,
>>       f = filename ? fopen(filename, "r") : NULL;
>>       g_free(filename);
>>       if (!f) {
>> -        fprintf(stderr, "Could not read keymap file: '%s'\n", language);
>> +        error_setg(errp, "could not read keymap file: '%s'", language);
>>           return NULL;
>>       }
>>   
>> @@ -118,7 +119,7 @@ static kbd_layout_t *parse_keyboard_layout(const name2keysym_t *table,
>>               continue;
>>           }
>>           if (!strncmp(line, "include ", 8)) {
>> -            parse_keyboard_layout(table, line + 8, k);
>> +            parse_keyboard_layout(table, line + 8, k, errp);
> Lacks error checking.
>
> Before this patch, an error in an included file is reported to stderr,
> but is otherwise ignored:
>
>      $ cat kbd-layout
>      include bad
>      include worse
>      $ qemu-system-x86_64 -nodefaults -S -monitor stdio -display vnc=:0 -k kbd-layout
>      QEMU 3.0.50 monitor - type 'help' for more information
>      (qemu) Could not read keymap file: 'bad'
>      Could not read keymap file: 'worse'
>
> Afterwards, the error gets stored in errp.  If a second include fails,
> we attempt to store another one, which fails the error_setv()'s
> assertion:
>
>      $ qemu-system-x86_64 -nodefaults -S -display vnc=:0 -k kbd-layout
>      qemu-system-x86_64: /work/armbru/qemu/util/error.c:57: error_setv: Assertion `*errp == NULL' failed.
>      Aborted (core dumped)
>
> If it's the only failed include, parse_keyboard_layout() sets an error
> and returns succes, which is a no-no.  Callers using the return value to
> determine success are prone to leak the error, or run into
> error_setv()'s assertion.  Callers using the error are prone to leak the
> returned kbd_layout_t.
Oh, sorry! My fault.. I mistook the passed errp in 
parse_keyboard_layout() for &error_fatal,
in this case once the first fails, qemu will exit(1) directly and no 
second include will be read.
But the truth is we use a *local_err=NULL in vnc_init_func to propagate 
to the &error_abort.
> Let's address this in a separate patch before this one.
>
>                 if (!parse_keyboard_layout(table, line + 8, k, errp))
Ok.
I have another thought, that is pass the &error_fatal directly to 
vnc_display_init/open(),
and exit() in the callee parse_keyboard_layout. But the side effect is 
drooping the obvious
error message: "Failed to init/start VNC server: ".
>
>>           } else {
>>               int offset = 0;
>>               while (line[offset] != 0 &&
>> @@ -170,9 +171,9 @@ static kbd_layout_t *parse_keyboard_layout(const name2keysym_t *table,
>>   
>>   
>>   kbd_layout_t *init_keyboard_layout(const name2keysym_t *table,
>> -                                   const char *language)
>> +                                   const char *language, Error **errp)
>>   {
>> -    return parse_keyboard_layout(table, language, NULL);
>> +    return parse_keyboard_layout(table, language, NULL, errp);
>>   }
>>   
>>   
>> diff --git a/ui/keymaps.h b/ui/keymaps.h
>> index 0693588225..98213a4191 100644
>> --- a/ui/keymaps.h
>> +++ b/ui/keymaps.h
>> @@ -53,7 +53,7 @@ typedef struct {
>>   typedef struct kbd_layout_t kbd_layout_t;
>>   
>>   kbd_layout_t *init_keyboard_layout(const name2keysym_t *table,
>> -                                   const char *language);
>> +                                   const char *language, Error **errp);
>>   int keysym2scancode(kbd_layout_t *k, int keysym,
>>                       bool shift, bool altgr, bool ctrl);
>>   int keycode_is_keypad(kbd_layout_t *k, int keycode);
>> diff --git a/ui/sdl.c b/ui/sdl.c
>> index a5fd503c25..b33df70f4d 100644
>> --- a/ui/sdl.c
>> +++ b/ui/sdl.c
>> @@ -29,6 +29,7 @@
>>   #include <SDL.h>
>>   #include <SDL_syswm.h>
>>   
>> +#include "qapi/error.h"
>>   #include "qemu-common.h"
>>   #include "qemu/cutils.h"
>>   #include "ui/console.h"
>> @@ -917,7 +918,8 @@ static void sdl1_display_init(DisplayState *ds, DisplayOptions *o)
>>           keyboard_layout = "en-us";
>>   #endif
>>       if(keyboard_layout) {
>> -        kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout);
>> +        kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout,
>> +                                          &error_fatal);
>>           if (!kbd_layout)
>>               exit(1);
> Again, exit() is now unreachable.
Yes.
>>       }
>> diff --git a/ui/vnc.c b/ui/vnc.c
>> index cf221c83cc..03c70b16f6 100644
>> --- a/ui/vnc.c
>> +++ b/ui/vnc.c
>> @@ -3205,7 +3205,7 @@ static const DisplayChangeListenerOps dcl_ops = {
>>       .dpy_cursor_define    = vnc_dpy_cursor_define,
>>   };
>>   
>> -void vnc_display_init(const char *id)
>> +void vnc_display_init(const char *id, Error **errp)
>>   {
>>       VncDisplay *vd;
>>   
>> @@ -3222,13 +3222,14 @@ void vnc_display_init(const char *id)
>>   
>>       if (keyboard_layout) {
>>           trace_vnc_key_map_init(keyboard_layout);
>> -        vd->kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout);
>> +        vd->kbd_layout = init_keyboard_layout(name2keysym,
>> +                                              keyboard_layout, errp);
>>       } else {
>> -        vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us");
>> +        vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us", errp);
>>       }
>>   
>>       if (!vd->kbd_layout) {
>> -        exit(1);
>> +        return;
>>       }
>>   
>>       vd->share_policy = VNC_SHARE_POLICY_ALLOW_EXCLUSIVE;
>> @@ -4079,7 +4080,12 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
>>       char *id = (char *)qemu_opts_id(opts);
>>   
>>       assert(id);
>> -    vnc_display_init(id);
>> +    vnc_display_init(id, &local_err);
>> +    if (local_err) {
>> +        error_prepend(&local_err, "Failed to init VNC server: ");
>> +        error_propagate(errp, local_err);
>> +        return -1;
>> +    }
> Works, but is inconsistent with the handling of vnc_display_open()
> errors, visible below.  Let's match that, and convert both to
> error_propagate() in a followup patch.
>
>>       vnc_display_open(id, &local_err);
>>       if (local_err != NULL) {
>>           error_reportf_err(local_err, "Failed to start VNC server: ");
> I think the easiest way forward is for me to pick a touched up version
> of this patch into a v2 of my "Replace some unwise uses of
> error_report() & friends" series.  You'll receive a copy, of course.
>
> Thanks!
Deal, that will be consistent and much easier. :)

Have a nice day, thanks
Fei

Patch

diff --git a/include/ui/console.h b/include/ui/console.h
index fb969caf70..c17803c530 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -453,7 +453,7 @@  void qemu_display_early_init(DisplayOptions *opts);
 void qemu_display_init(DisplayState *ds, DisplayOptions *opts);
 
 /* vnc.c */
-void vnc_display_init(const char *id);
+void vnc_display_init(const char *id, Error **errp);
 void vnc_display_open(const char *id, Error **errp);
 void vnc_display_add_client(const char *id, int csock, bool skipauth);
 int vnc_display_password(const char *id, const char *password);
diff --git a/ui/curses.c b/ui/curses.c
index 59d819fd4d..60c0962334 100644
--- a/ui/curses.c
+++ b/ui/curses.c
@@ -28,6 +28,7 @@ 
 #include <termios.h>
 #endif
 
+#include "qapi/error.h"
 #include "qemu-common.h"
 #include "ui/console.h"
 #include "ui/input.h"
@@ -421,7 +422,8 @@  static void curses_keyboard_setup(void)
         keyboard_layout = "en-us";
 #endif
     if(keyboard_layout) {
-        kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout);
+        kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout,
+                                          &error_fatal);
         if (!kbd_layout)
             exit(1);
     }
diff --git a/ui/keymaps.c b/ui/keymaps.c
index 43fe604724..52d432e856 100644
--- a/ui/keymaps.c
+++ b/ui/keymaps.c
@@ -27,6 +27,7 @@ 
 #include "sysemu/sysemu.h"
 #include "trace.h"
 #include "qemu/error-report.h"
+#include "qapi/error.h"
 
 struct keysym2code {
     uint32_t count;
@@ -81,7 +82,7 @@  static void add_keysym(char *line, int keysym, int keycode, kbd_layout_t *k)
 
 static kbd_layout_t *parse_keyboard_layout(const name2keysym_t *table,
                                            const char *language,
-                                           kbd_layout_t *k)
+                                           kbd_layout_t *k, Error **errp)
 {
     FILE *f;
     char * filename;
@@ -94,7 +95,7 @@  static kbd_layout_t *parse_keyboard_layout(const name2keysym_t *table,
     f = filename ? fopen(filename, "r") : NULL;
     g_free(filename);
     if (!f) {
-        fprintf(stderr, "Could not read keymap file: '%s'\n", language);
+        error_setg(errp, "could not read keymap file: '%s'", language);
         return NULL;
     }
 
@@ -118,7 +119,7 @@  static kbd_layout_t *parse_keyboard_layout(const name2keysym_t *table,
             continue;
         }
         if (!strncmp(line, "include ", 8)) {
-            parse_keyboard_layout(table, line + 8, k);
+            parse_keyboard_layout(table, line + 8, k, errp);
         } else {
             int offset = 0;
             while (line[offset] != 0 &&
@@ -170,9 +171,9 @@  static kbd_layout_t *parse_keyboard_layout(const name2keysym_t *table,
 
 
 kbd_layout_t *init_keyboard_layout(const name2keysym_t *table,
-                                   const char *language)
+                                   const char *language, Error **errp)
 {
-    return parse_keyboard_layout(table, language, NULL);
+    return parse_keyboard_layout(table, language, NULL, errp);
 }
 
 
diff --git a/ui/keymaps.h b/ui/keymaps.h
index 0693588225..98213a4191 100644
--- a/ui/keymaps.h
+++ b/ui/keymaps.h
@@ -53,7 +53,7 @@  typedef struct {
 typedef struct kbd_layout_t kbd_layout_t;
 
 kbd_layout_t *init_keyboard_layout(const name2keysym_t *table,
-                                   const char *language);
+                                   const char *language, Error **errp);
 int keysym2scancode(kbd_layout_t *k, int keysym,
                     bool shift, bool altgr, bool ctrl);
 int keycode_is_keypad(kbd_layout_t *k, int keycode);
diff --git a/ui/sdl.c b/ui/sdl.c
index a5fd503c25..b33df70f4d 100644
--- a/ui/sdl.c
+++ b/ui/sdl.c
@@ -29,6 +29,7 @@ 
 #include <SDL.h>
 #include <SDL_syswm.h>
 
+#include "qapi/error.h"
 #include "qemu-common.h"
 #include "qemu/cutils.h"
 #include "ui/console.h"
@@ -917,7 +918,8 @@  static void sdl1_display_init(DisplayState *ds, DisplayOptions *o)
         keyboard_layout = "en-us";
 #endif
     if(keyboard_layout) {
-        kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout);
+        kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout,
+                                          &error_fatal);
         if (!kbd_layout)
             exit(1);
     }
diff --git a/ui/vnc.c b/ui/vnc.c
index cf221c83cc..03c70b16f6 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3205,7 +3205,7 @@  static const DisplayChangeListenerOps dcl_ops = {
     .dpy_cursor_define    = vnc_dpy_cursor_define,
 };
 
-void vnc_display_init(const char *id)
+void vnc_display_init(const char *id, Error **errp)
 {
     VncDisplay *vd;
 
@@ -3222,13 +3222,14 @@  void vnc_display_init(const char *id)
 
     if (keyboard_layout) {
         trace_vnc_key_map_init(keyboard_layout);
-        vd->kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout);
+        vd->kbd_layout = init_keyboard_layout(name2keysym,
+                                              keyboard_layout, errp);
     } else {
-        vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us");
+        vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us", errp);
     }
 
     if (!vd->kbd_layout) {
-        exit(1);
+        return;
     }
 
     vd->share_policy = VNC_SHARE_POLICY_ALLOW_EXCLUSIVE;
@@ -4079,7 +4080,12 @@  int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
     char *id = (char *)qemu_opts_id(opts);
 
     assert(id);
-    vnc_display_init(id);
+    vnc_display_init(id, &local_err);
+    if (local_err) {
+        error_prepend(&local_err, "Failed to init VNC server: ");
+        error_propagate(errp, local_err);
+        return -1;
+    }
     vnc_display_open(id, &local_err);
     if (local_err != NULL) {
         error_reportf_err(local_err, "Failed to start VNC server: ");