| Message ID | 20181015115309.17089-34-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Replace some unwise uses of error_report() & friends | expand |
On 15.10.18 13:53, Markus Armbruster wrote: > Calling error_report() from within a function that takes an Error ** > argument is suspicious. drive_new() calls error_report() even though > it can run within drive_init_func(), which takes an Error ** argument. > drive_init_func()'s caller main(), via qemu_opts_foreach(), is fine > with it, but clean it up anyway: > > * Convert drive_new() to Error > > * Update add_init_drive() to report the error received from > drive_new() > > * Make main() pass &error_fatal through qemu_opts_foreach(), > drive_init_func() to drive_new() > > * Make default_drive() pass &error_abort through qemu_opts_foreach(), > drive_init_func() to drive_new() > > Cc: Kevin Wolf <kwolf@redhat.com> > Cc: Max Reitz <mreitz@redhat.com> > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > blockdev.c | 27 ++++++++++++++------------- > device-hotplug.c | 5 ++++- > include/sysemu/blockdev.h | 3 ++- > vl.c | 8 ++++---- > 4 files changed, 24 insertions(+), 19 deletions(-) [...] > diff --git a/vl.c b/vl.c > index 65366b961e..22beca29d1 100644 > --- a/vl.c > +++ b/vl.c [...] > @@ -4396,7 +4395,8 @@ int main(int argc, char **argv, char **envp) > NULL, NULL); > } > if (qemu_opts_foreach(qemu_find_opts("drive"), drive_init_func, > - &machine_class->block_default_type, NULL)) { > + &machine_class->block_default_type, &error_fatal)) { > + /* We printed help */ > exit(1); > } I thought you wanted it to become an exit(0)? I don't care either way, though, so: Reviewed-by: Max Reitz <mreitz@redhat.com>
On 10/15/18 9:48 AM, Max Reitz wrote: > On 15.10.18 13:53, Markus Armbruster wrote: >> Calling error_report() from within a function that takes an Error ** >> argument is suspicious. drive_new() calls error_report() even though >> it can run within drive_init_func(), which takes an Error ** argument. >> drive_init_func()'s caller main(), via qemu_opts_foreach(), is fine >> with it, but clean it up anyway: >> >> @@ -4396,7 +4395,8 @@ int main(int argc, char **argv, char **envp) >> NULL, NULL); >> } >> if (qemu_opts_foreach(qemu_find_opts("drive"), drive_init_func, >> - &machine_class->block_default_type, NULL)) { >> + &machine_class->block_default_type, &error_fatal)) { >> + /* We printed help */ >> exit(1); >> } > > I thought you wanted it to become an exit(0)? I don't care either way, > though, so: > > Reviewed-by: Max Reitz <mreitz@redhat.com> I _do_ care. Printing help isn't an error, so it shouldn't result in a non-zero exit status.
On 15/10/2018 16:48, Max Reitz wrote: > On 15.10.18 13:53, Markus Armbruster wrote: >> Calling error_report() from within a function that takes an Error ** >> argument is suspicious. drive_new() calls error_report() even though >> it can run within drive_init_func(), which takes an Error ** argument. >> drive_init_func()'s caller main(), via qemu_opts_foreach(), is fine >> with it, but clean it up anyway: >> >> * Convert drive_new() to Error >> >> * Update add_init_drive() to report the error received from >> drive_new() >> >> * Make main() pass &error_fatal through qemu_opts_foreach(), >> drive_init_func() to drive_new() >> >> * Make default_drive() pass &error_abort through qemu_opts_foreach(), >> drive_init_func() to drive_new() >> >> Cc: Kevin Wolf <kwolf@redhat.com> >> Cc: Max Reitz <mreitz@redhat.com> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> blockdev.c | 27 ++++++++++++++------------- >> device-hotplug.c | 5 ++++- >> include/sysemu/blockdev.h | 3 ++- >> vl.c | 8 ++++---- >> 4 files changed, 24 insertions(+), 19 deletions(-) > > [...] > >> diff --git a/vl.c b/vl.c >> index 65366b961e..22beca29d1 100644 >> --- a/vl.c >> +++ b/vl.c > > [...] >> @@ -4396,7 +4395,8 @@ int main(int argc, char **argv, char **envp) >> NULL, NULL); >> } >> if (qemu_opts_foreach(qemu_find_opts("drive"), drive_init_func, >> - &machine_class->block_default_type, NULL)) { >> + &machine_class->block_default_type, &error_fatal)) { >> + /* We printed help */ >> exit(1); >> } > > I thought you wanted it to become an exit(0)? I don't care either way, Markus did it in the next patch ;) Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > though, so: > > Reviewed-by: Max Reitz <mreitz@redhat.com> >
Philippe Mathieu-Daudé <philmd@redhat.com> writes: > On 15/10/2018 16:48, Max Reitz wrote: >> On 15.10.18 13:53, Markus Armbruster wrote: >>> Calling error_report() from within a function that takes an Error ** >>> argument is suspicious. drive_new() calls error_report() even though >>> it can run within drive_init_func(), which takes an Error ** argument. >>> drive_init_func()'s caller main(), via qemu_opts_foreach(), is fine >>> with it, but clean it up anyway: >>> >>> * Convert drive_new() to Error >>> >>> * Update add_init_drive() to report the error received from >>> drive_new() >>> >>> * Make main() pass &error_fatal through qemu_opts_foreach(), >>> drive_init_func() to drive_new() >>> >>> * Make default_drive() pass &error_abort through qemu_opts_foreach(), >>> drive_init_func() to drive_new() >>> >>> Cc: Kevin Wolf <kwolf@redhat.com> >>> Cc: Max Reitz <mreitz@redhat.com> >>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>> --- >>> blockdev.c | 27 ++++++++++++++------------- >>> device-hotplug.c | 5 ++++- >>> include/sysemu/blockdev.h | 3 ++- >>> vl.c | 8 ++++---- >>> 4 files changed, 24 insertions(+), 19 deletions(-) >> >> [...] >> >>> diff --git a/vl.c b/vl.c >>> index 65366b961e..22beca29d1 100644 >>> --- a/vl.c >>> +++ b/vl.c >> >> [...] >>> @@ -4396,7 +4395,8 @@ int main(int argc, char **argv, char **envp) >>> NULL, NULL); >>> } >>> if (qemu_opts_foreach(qemu_find_opts("drive"), drive_init_func, >>> - &machine_class->block_default_type, NULL)) { >>> + &machine_class->block_default_type, &error_fatal)) { >>> + /* We printed help */ >>> exit(1); >>> } >> >> I thought you wanted it to become an exit(0)? I don't care either way, > > Markus did it in the next patch ;) I noticed I was about to start a commit message paragraph with "Also", and split the patch :) > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Thanks!
diff --git a/blockdev.c b/blockdev.c index a8755bd908..574adbcb7f 100644 --- a/blockdev.c +++ b/blockdev.c @@ -759,7 +759,8 @@ QemuOptsList qemu_legacy_drive_opts = { }, }; -DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type) +DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type, + Error **errp) { const char *value; BlockBackend *blk; @@ -808,7 +809,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type) qemu_opt_rename(all_opts, opt_renames[i].from, opt_renames[i].to, &local_err); if (local_err) { - error_report_err(local_err); + error_propagate(errp, local_err); return NULL; } } @@ -819,7 +820,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type) bool writethrough; if (bdrv_parse_cache_mode(value, &flags, &writethrough) != 0) { - error_report("invalid cache option"); + error_setg(errp, "invalid cache option"); return NULL; } @@ -847,7 +848,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type) &error_abort); qemu_opts_absorb_qdict(legacy_opts, bs_opts, &local_err); if (local_err) { - error_report_err(local_err); + error_propagate(errp, local_err); goto fail; } @@ -860,7 +861,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type) media = MEDIA_CDROM; read_only = true; } else { - error_report("'%s' invalid media", value); + error_setg(errp, "'%s' invalid media", value); goto fail; } } @@ -885,7 +886,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type) type++) { } if (type == IF_COUNT) { - error_report("unsupported bus type '%s'", value); + error_setg(errp, "unsupported bus type '%s'", value); goto fail; } } else { @@ -902,7 +903,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type) if (index != -1) { if (bus_id != 0 || unit_id != -1) { - error_report("index cannot be used with bus and unit"); + error_setg(errp, "index cannot be used with bus and unit"); goto fail; } bus_id = drive_index_to_bus_id(type, index); @@ -921,13 +922,13 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type) } if (max_devs && unit_id >= max_devs) { - error_report("unit %d too big (max is %d)", unit_id, max_devs - 1); + error_setg(errp, "unit %d too big (max is %d)", unit_id, max_devs - 1); goto fail; } if (drive_get(type, bus_id, unit_id) != NULL) { - error_report("drive with bus=%d, unit=%d (index=%d) exists", - bus_id, unit_id, index); + error_setg(errp, "drive with bus=%d, unit=%d (index=%d) exists", + bus_id, unit_id, index); goto fail; } @@ -970,7 +971,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type) if (werror != NULL) { if (type != IF_IDE && type != IF_SCSI && type != IF_VIRTIO && type != IF_NONE) { - error_report("werror is not supported by this bus type"); + error_setg(errp, "werror is not supported by this bus type"); goto fail; } qdict_put_str(bs_opts, "werror", werror); @@ -980,7 +981,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type) if (rerror != NULL) { if (type != IF_IDE && type != IF_VIRTIO && type != IF_SCSI && type != IF_NONE) { - error_report("rerror is not supported by this bus type"); + error_setg(errp, "rerror is not supported by this bus type"); goto fail; } qdict_put_str(bs_opts, "rerror", rerror); @@ -991,7 +992,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type) bs_opts = NULL; if (!blk) { if (local_err) { - error_report_err(local_err); + error_propagate(errp, local_err); } goto fail; } else { diff --git a/device-hotplug.c b/device-hotplug.c index cd427e2c76..6090d5f1e9 100644 --- a/device-hotplug.c +++ b/device-hotplug.c @@ -28,6 +28,7 @@ #include "sysemu/block-backend.h" #include "sysemu/blockdev.h" #include "qapi/qmp/qdict.h" +#include "qapi/error.h" #include "qemu/config-file.h" #include "qemu/option.h" #include "sysemu/sysemu.h" @@ -36,6 +37,7 @@ static DriveInfo *add_init_drive(const char *optstr) { + Error *err = NULL; DriveInfo *dinfo; QemuOpts *opts; MachineClass *mc; @@ -45,8 +47,9 @@ static DriveInfo *add_init_drive(const char *optstr) return NULL; mc = MACHINE_GET_CLASS(current_machine); - dinfo = drive_new(opts, mc->block_default_type); + dinfo = drive_new(opts, mc->block_default_type, &err); if (!dinfo) { + error_report_err(err); qemu_opts_del(opts); return NULL; } diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h index 24954b94e0..d34c4920dc 100644 --- a/include/sysemu/blockdev.h +++ b/include/sysemu/blockdev.h @@ -54,7 +54,8 @@ DriveInfo *drive_get_next(BlockInterfaceType type); QemuOpts *drive_def(const char *optstr); QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file, const char *optstr); -DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type); +DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type, + Error **errp); /* device-hotplug */ diff --git a/vl.c b/vl.c index 65366b961e..22beca29d1 100644 --- a/vl.c +++ b/vl.c @@ -1129,7 +1129,7 @@ static int drive_init_func(void *opaque, QemuOpts *opts, Error **errp) { BlockInterfaceType *block_default_type = opaque; - return drive_new(opts, *block_default_type) == NULL; + return drive_new(opts, *block_default_type, errp) == NULL; } static int drive_enable_snapshot(void *opaque, QemuOpts *opts, Error **errp) @@ -1155,8 +1155,7 @@ static void default_drive(int enable, int snapshot, BlockInterfaceType type, drive_enable_snapshot(NULL, opts, NULL); } - dinfo = drive_new(opts, type); - assert(dinfo); + dinfo = drive_new(opts, type, &error_abort); dinfo->is_default = true; } @@ -4396,7 +4395,8 @@ int main(int argc, char **argv, char **envp) NULL, NULL); } if (qemu_opts_foreach(qemu_find_opts("drive"), drive_init_func, - &machine_class->block_default_type, NULL)) { + &machine_class->block_default_type, &error_fatal)) { + /* We printed help */ exit(1); }
Calling error_report() from within a function that takes an Error ** argument is suspicious. drive_new() calls error_report() even though it can run within drive_init_func(), which takes an Error ** argument. drive_init_func()'s caller main(), via qemu_opts_foreach(), is fine with it, but clean it up anyway: * Convert drive_new() to Error * Update add_init_drive() to report the error received from drive_new() * Make main() pass &error_fatal through qemu_opts_foreach(), drive_init_func() to drive_new() * Make default_drive() pass &error_abort through qemu_opts_foreach(), drive_init_func() to drive_new() Cc: Kevin Wolf <kwolf@redhat.com> Cc: Max Reitz <mreitz@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com> --- blockdev.c | 27 ++++++++++++++------------- device-hotplug.c | 5 ++++- include/sysemu/blockdev.h | 3 ++- vl.c | 8 ++++---- 4 files changed, 24 insertions(+), 19 deletions(-)