| Message ID | 20181015115309.17089-28-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Replace some unwise uses of error_report() & friends | expand |
On 10/15/2018 07:53 PM, Markus Armbruster wrote: > Calling error_report() in a function that takes an Error ** argument > is suspicious. vnc_init_func() does that, and then fails without > setting an error. Its caller main(), via qemu_opts_foreach(), is fine > with it, but clean it up anyway. > > Cc: Gerd Hoffmann <kraxel@redhat.com> > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > ui/vnc.c | 9 +++++---- > vl.c | 2 +- > 2 files changed, 6 insertions(+), 5 deletions(-) > > diff --git a/ui/vnc.c b/ui/vnc.c > index 98e3d3b1d8..fcd2744d52 100644 > --- a/ui/vnc.c > +++ b/ui/vnc.c > @@ -4082,13 +4082,14 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp) > assert(id); > vnc_display_init(id, &local_err); > if (local_err) { > - error_report_err(local_err); > - exit(1); > + error_propagate(errp, local_err); Shall we use error_propagate(errp, local_err, ("Failed to init VNC server: "); like vnc_display_open does? If yes, I guess this error message should be added from last patch [26/35]. If not, please omit. :) Have a nice day Fei > + return -1; > } > vnc_display_open(id, &local_err); > if (local_err != NULL) { > - error_reportf_err(local_err, "Failed to start VNC server: "); > - exit(1); > + error_propagate_prepend(errp, local_err, > + "Failed to start VNC server: "); > + return -1; > } > return 0; > } > diff --git a/vl.c b/vl.c > index c053117028..8e0006d49c 100644 > --- a/vl.c > +++ b/vl.c > @@ -4526,7 +4526,7 @@ int main(int argc, char **argv, char **envp) > /* init remote displays */ > #ifdef CONFIG_VNC > qemu_opts_foreach(qemu_find_opts("vnc"), > - vnc_init_func, NULL, NULL); > + vnc_init_func, NULL, &error_fatal); > #endif > > if (using_spice) {
On 15/10/2018 13:53, Markus Armbruster wrote: > Calling error_report() in a function that takes an Error ** argument > is suspicious. vnc_init_func() does that, and then fails without > setting an error. Its caller main(), via qemu_opts_foreach(), is fine > with it, but clean it up anyway. > > Cc: Gerd Hoffmann <kraxel@redhat.com> > Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > ui/vnc.c | 9 +++++---- > vl.c | 2 +- > 2 files changed, 6 insertions(+), 5 deletions(-) > > diff --git a/ui/vnc.c b/ui/vnc.c > index 98e3d3b1d8..fcd2744d52 100644 > --- a/ui/vnc.c > +++ b/ui/vnc.c > @@ -4082,13 +4082,14 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp) > assert(id); > vnc_display_init(id, &local_err); > if (local_err) { > - error_report_err(local_err); > - exit(1); > + 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: "); > - exit(1); > + error_propagate_prepend(errp, local_err, > + "Failed to start VNC server: "); > + return -1; > } > return 0; > } > diff --git a/vl.c b/vl.c > index c053117028..8e0006d49c 100644 > --- a/vl.c > +++ b/vl.c > @@ -4526,7 +4526,7 @@ int main(int argc, char **argv, char **envp) > /* init remote displays */ > #ifdef CONFIG_VNC > qemu_opts_foreach(qemu_find_opts("vnc"), > - vnc_init_func, NULL, NULL); > + vnc_init_func, NULL, &error_fatal); > #endif > > if (using_spice) { >
Fei Li <fli@suse.com> writes: > On 10/15/2018 07:53 PM, Markus Armbruster wrote: >> Calling error_report() in a function that takes an Error ** argument >> is suspicious. vnc_init_func() does that, and then fails without >> setting an error. Its caller main(), via qemu_opts_foreach(), is fine >> with it, but clean it up anyway. >> >> Cc: Gerd Hoffmann <kraxel@redhat.com> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> ui/vnc.c | 9 +++++---- >> vl.c | 2 +- >> 2 files changed, 6 insertions(+), 5 deletions(-) >> >> diff --git a/ui/vnc.c b/ui/vnc.c >> index 98e3d3b1d8..fcd2744d52 100644 >> --- a/ui/vnc.c >> +++ b/ui/vnc.c >> @@ -4082,13 +4082,14 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp) >> assert(id); >> vnc_display_init(id, &local_err); >> if (local_err) { >> - error_report_err(local_err); >> - exit(1); >> + error_propagate(errp, local_err); > Shall we use error_propagate(errp, local_err, ("Failed to init VNC > server: "); > like vnc_display_open does? I don't know. The error reporting is somewhat poor around here. Consider: $ qemu-system-x86_64 -display vnc=:0,share=nope qemu-system-x86_64: -display vnc=:0,share=nope: Failed to start VNC server: unknown vnc share= option The "Failed to start VNC server: " prefix doesn't really add value. $ qemu-system-x86_64 -display vnc=:0 -k bad qemu-system-x86_64: -display vnc=:0: could not read keymap file: 'bad' The error message refers to -display instead of -k. I doubt adding the prefix would improve it. Gerd, what do you think? > If yes, I guess this error message should be added from last patch [26/35]. Yes. > If not, please omit. :) > > Have a nice day > Fei >> + return -1; >> } >> vnc_display_open(id, &local_err); >> if (local_err != NULL) { >> - error_reportf_err(local_err, "Failed to start VNC server: "); >> - exit(1); >> + error_propagate_prepend(errp, local_err, >> + "Failed to start VNC server: "); >> + return -1; >> } >> return 0; >> } >> diff --git a/vl.c b/vl.c >> index c053117028..8e0006d49c 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -4526,7 +4526,7 @@ int main(int argc, char **argv, char **envp) >> /* init remote displays */ >> #ifdef CONFIG_VNC >> qemu_opts_foreach(qemu_find_opts("vnc"), >> - vnc_init_func, NULL, NULL); >> + vnc_init_func, NULL, &error_fatal); >> #endif >> if (using_spice) {
> >> + error_propagate(errp, local_err); > > Shall we use error_propagate(errp, local_err, ("Failed to init VNC > > server: "); > > like vnc_display_open does? > > I don't know. > > The error reporting is somewhat poor around here. Consider: > > $ qemu-system-x86_64 -display vnc=:0,share=nope > qemu-system-x86_64: -display vnc=:0,share=nope: Failed to start VNC server: unknown vnc share= option > > The "Failed to start VNC server: " prefix doesn't really add value. Indeed. > Gerd, what do you think? I'd keep the messages short. cheers, Gerd
Gerd Hoffmann <kraxel@redhat.com> writes: >> >> + error_propagate(errp, local_err); >> > Shall we use error_propagate(errp, local_err, ("Failed to init VNC >> > server: "); >> > like vnc_display_open does? >> >> I don't know. >> >> The error reporting is somewhat poor around here. Consider: >> >> $ qemu-system-x86_64 -display vnc=:0,share=nope >> qemu-system-x86_64: -display vnc=:0,share=nope: Failed to start VNC server: unknown vnc share= option >> >> The "Failed to start VNC server: " prefix doesn't really add value. > > Indeed. > >> Gerd, what do you think? > > I'd keep the messages short. Okay, I'll delete the prefix. Thanks!
diff --git a/ui/vnc.c b/ui/vnc.c index 98e3d3b1d8..fcd2744d52 100644 --- a/ui/vnc.c +++ b/ui/vnc.c @@ -4082,13 +4082,14 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp) assert(id); vnc_display_init(id, &local_err); if (local_err) { - error_report_err(local_err); - exit(1); + 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: "); - exit(1); + error_propagate_prepend(errp, local_err, + "Failed to start VNC server: "); + return -1; } return 0; } diff --git a/vl.c b/vl.c index c053117028..8e0006d49c 100644 --- a/vl.c +++ b/vl.c @@ -4526,7 +4526,7 @@ int main(int argc, char **argv, char **envp) /* init remote displays */ #ifdef CONFIG_VNC qemu_opts_foreach(qemu_find_opts("vnc"), - vnc_init_func, NULL, NULL); + vnc_init_func, NULL, &error_fatal); #endif if (using_spice) {
Calling error_report() in a function that takes an Error ** argument is suspicious. vnc_init_func() does that, and then fails without setting an error. Its caller main(), via qemu_opts_foreach(), is fine with it, but clean it up anyway. Cc: Gerd Hoffmann <kraxel@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com> --- ui/vnc.c | 9 +++++---- vl.c | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-)