* Re: [Qemu-devel] [PATCH v2] ui/vnc.c: polish vnc_init_func
2018-10-12 11:50 [Qemu-devel] [PATCH v2] ui/vnc.c: polish vnc_init_func Fei Li
@ 2018-10-12 11:59 ` Fei Li
2018-10-12 12:42 ` Philippe Mathieu-Daudé
2018-10-15 7:58 ` Markus Armbruster
2 siblings, 0 replies; 5+ messages in thread
From: Fei Li @ 2018-10-12 11:59 UTC (permalink / raw)
To: qemu-devel, armbru
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: ");
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v2] ui/vnc.c: polish vnc_init_func
2018-10-12 11:50 [Qemu-devel] [PATCH v2] ui/vnc.c: polish vnc_init_func Fei Li
2018-10-12 11:59 ` Fei Li
@ 2018-10-12 12:42 ` Philippe Mathieu-Daudé
2018-10-15 7:58 ` Markus Armbruster
2 siblings, 0 replies; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-10-12 12:42 UTC (permalink / raw)
To: Fei Li, qemu-devel, armbru
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: ");
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v2] ui/vnc.c: polish vnc_init_func
2018-10-12 11:50 [Qemu-devel] [PATCH v2] ui/vnc.c: polish vnc_init_func Fei Li
2018-10-12 11:59 ` Fei Li
2018-10-12 12:42 ` Philippe Mathieu-Daudé
@ 2018-10-15 7:58 ` Markus Armbruster
2018-10-15 10:08 ` Fei Li
2 siblings, 1 reply; 5+ messages in thread
From: Markus Armbruster @ 2018-10-15 7:58 UTC (permalink / raw)
To: Fei Li; +Cc: qemu-devel
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!
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v2] ui/vnc.c: polish vnc_init_func
2018-10-15 7:58 ` Markus Armbruster
@ 2018-10-15 10:08 ` Fei Li
0 siblings, 0 replies; 5+ messages in thread
From: Fei Li @ 2018-10-15 10:08 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread