| Message ID | 20181012115013.29555-1-fli@suse.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] ui/vnc.c: polish vnc_init_func | expand |
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: ");
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: "); >
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!
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
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: ");
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(-)