| Message ID | 1434205258-1932-5-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 06/13/2015 08:20 AM, Markus Armbruster wrote: > Error classes other than ERROR_CLASS_GENERIC_ERROR should not be used > in new code. Hiding them in QERR_ macros makes new uses hard to spot. > Fortunately, there's just one such macro left. Eliminate it with this > coccinelle semantic patch: > > @@ > expression EP, E; > @@ > -error_set(EP, QERR_DEVICE_NOT_FOUND, E) > +error_set(EP, ERROR_CLASS_DEVICE_NOT_FOUND, "Device '%s' not found", E) > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > backends/rng-egd.c | 3 ++- > blockdev-nbd.c | 3 ++- > blockdev.c | 33 ++++++++++++++++++++++----------- > hmp.c | 6 ++++-- > include/qapi/qmp/qerror.h | 3 --- > net/net.c | 6 ++++-- > qdev-monitor.c | 6 ++++-- > qmp.c | 12 ++++++++---- > qom/object.c | 6 ++++-- > ui/input.c | 3 ++- > 10 files changed, 52 insertions(+), 29 deletions(-) Plain transformation would be closer to a 1:1 insertion/deletion count. The larger insertion count is due to reflowing long lines after the transformation. Does coccinelle do that for you, or do you have to touch things up manually? But I'm okay with the result. Reviewed-by: Eric Blake <eblake@redhat.com>
On Sat, Jun 13, 2015 at 04:20:51PM +0200, Markus Armbruster wrote: > Error classes other than ERROR_CLASS_GENERIC_ERROR should not be used > in new code. Hiding them in QERR_ macros makes new uses hard to spot. > Fortunately, there's just one such macro left. Eliminate it with this > coccinelle semantic patch: > > @@ > expression EP, E; > @@ > -error_set(EP, QERR_DEVICE_NOT_FOUND, E) > +error_set(EP, ERROR_CLASS_DEVICE_NOT_FOUND, "Device '%s' not found", E) > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > backends/rng-egd.c | 3 ++- > blockdev-nbd.c | 3 ++- > blockdev.c | 33 ++++++++++++++++++++++----------- > hmp.c | 6 ++++-- > include/qapi/qmp/qerror.h | 3 --- > net/net.c | 6 ++++-- > qdev-monitor.c | 6 ++++-- > qmp.c | 12 ++++++++---- > qom/object.c | 6 ++++-- > ui/input.c | 3 ++- > 10 files changed, 52 insertions(+), 29 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
On Sat, 13 Jun 2015 16:20:51 +0200 Markus Armbruster <armbru@redhat.com> wrote: > Error classes other than ERROR_CLASS_GENERIC_ERROR should not be used > in new code. Hiding them in QERR_ macros makes new uses hard to spot. > Fortunately, there's just one such macro left. Eliminate it with this > coccinelle semantic patch: > > @@ > expression EP, E; > @@ > -error_set(EP, QERR_DEVICE_NOT_FOUND, E) > +error_set(EP, ERROR_CLASS_DEVICE_NOT_FOUND, "Device '%s' not found", E) This is a bit minor, but I think I'd have created a new function instead, say error_set_enodev(). This avoids all the duplication. But I'm not asking you to change, as the patch is good and this can be done in the future if we so want. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > backends/rng-egd.c | 3 ++- > blockdev-nbd.c | 3 ++- > blockdev.c | 33 ++++++++++++++++++++++----------- > hmp.c | 6 ++++-- > include/qapi/qmp/qerror.h | 3 --- > net/net.c | 6 ++++-- > qdev-monitor.c | 6 ++++-- > qmp.c | 12 ++++++++---- > qom/object.c | 6 ++++-- > ui/input.c | 3 ++- > 10 files changed, 52 insertions(+), 29 deletions(-) > > diff --git a/backends/rng-egd.c b/backends/rng-egd.c > index 2962795..849bd7a 100644 > --- a/backends/rng-egd.c > +++ b/backends/rng-egd.c > @@ -147,7 +147,8 @@ static void rng_egd_opened(RngBackend *b, Error **errp) > > s->chr = qemu_chr_find(s->chr_name); > if (s->chr == NULL) { > - error_set(errp, QERR_DEVICE_NOT_FOUND, s->chr_name); > + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, > + "Device '%s' not found", s->chr_name); > return; > } > > diff --git a/blockdev-nbd.c b/blockdev-nbd.c > index 0d9df47..128e810 100644 > --- a/blockdev-nbd.c > +++ b/blockdev-nbd.c > @@ -91,7 +91,8 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable, > > blk = blk_by_name(device); > if (!blk) { > - error_set(errp, QERR_DEVICE_NOT_FOUND, device); > + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, > + "Device '%s' not found", device); > return; > } > if (!blk_is_inserted(blk)) { > diff --git a/blockdev.c b/blockdev.c > index a88ea76..8dec0cc 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1102,7 +1102,8 @@ SnapshotInfo *qmp_blockdev_snapshot_delete_internal_sync(const char *device, > > blk = blk_by_name(device); > if (!blk) { > - error_set(errp, QERR_DEVICE_NOT_FOUND, device); > + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, > + "Device '%s' not found", device); > return NULL; > } > bs = blk_bs(blk); > @@ -1291,7 +1292,8 @@ static void internal_snapshot_prepare(BlkTransactionState *common, > /* 2. check for validation */ > blk = blk_by_name(device); > if (!blk) { > - error_set(errp, QERR_DEVICE_NOT_FOUND, device); > + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, > + "Device '%s' not found", device); > return; > } > bs = blk_bs(blk); > @@ -1571,7 +1573,8 @@ static void drive_backup_prepare(BlkTransactionState *common, Error **errp) > > blk = blk_by_name(backup->device); > if (!blk) { > - error_set(errp, QERR_DEVICE_NOT_FOUND, backup->device); > + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, > + "Device '%s' not found", backup->device); > return; > } > bs = blk_bs(blk); > @@ -1841,7 +1844,8 @@ void qmp_eject(const char *device, bool has_force, bool force, Error **errp) > > blk = blk_by_name(device); > if (!blk) { > - error_set(errp, QERR_DEVICE_NOT_FOUND, device); > + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, > + "Device '%s' not found", device); > return; > } > > @@ -1901,7 +1905,8 @@ void qmp_change_blockdev(const char *device, const char *filename, > > blk = blk_by_name(device); > if (!blk) { > - error_set(errp, QERR_DEVICE_NOT_FOUND, device); > + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, > + "Device '%s' not found", device); > return; > } > bs = blk_bs(blk); > @@ -1960,7 +1965,8 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd, > > blk = blk_by_name(device); > if (!blk) { > - error_set(errp, QERR_DEVICE_NOT_FOUND, device); > + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, > + "Device '%s' not found", device); > return; > } > bs = blk_bs(blk); > @@ -2275,7 +2281,8 @@ void qmp_block_stream(const char *device, > > blk = blk_by_name(device); > if (!blk) { > - error_set(errp, QERR_DEVICE_NOT_FOUND, device); > + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, > + "Device '%s' not found", device); > return; > } > bs = blk_bs(blk); > @@ -2349,7 +2356,8 @@ void qmp_block_commit(const char *device, > * scenario in which all optional arguments are omitted. */ > blk = blk_by_name(device); > if (!blk) { > - error_set(errp, QERR_DEVICE_NOT_FOUND, device); > + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, > + "Device '%s' not found", device); > return; > } > bs = blk_bs(blk); > @@ -2461,7 +2469,8 @@ void qmp_drive_backup(const char *device, const char *target, > > blk = blk_by_name(device); > if (!blk) { > - error_set(errp, QERR_DEVICE_NOT_FOUND, device); > + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, > + "Device '%s' not found", device); > return; > } > bs = blk_bs(blk); > @@ -2675,7 +2684,8 @@ void qmp_drive_mirror(const char *device, const char *target, > > blk = blk_by_name(device); > if (!blk) { > - error_set(errp, QERR_DEVICE_NOT_FOUND, device); > + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, > + "Device '%s' not found", device); > return; > } > bs = blk_bs(blk); > @@ -2941,7 +2951,8 @@ void qmp_change_backing_file(const char *device, > > blk = blk_by_name(device); > if (!blk) { > - error_set(errp, QERR_DEVICE_NOT_FOUND, device); > + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, > + "Device '%s' not found", device); > return; > } > bs = blk_bs(blk); > diff --git a/hmp.c b/hmp.c > index 05e4927..6f63927 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -1863,7 +1863,8 @@ void hmp_qemu_io(Monitor *mon, const QDict *qdict) > if (blk) { > qemuio_command(blk, command); > } else { > - error_set(&err, QERR_DEVICE_NOT_FOUND, device); > + error_set(&err, ERROR_CLASS_DEVICE_NOT_FOUND, > + "Device '%s' not found", device); > } > > hmp_handle_error(mon, &err); > @@ -1991,7 +1992,8 @@ void hmp_qom_set(Monitor *mon, const QDict *qdict) > > obj = object_resolve_path(path, &ambiguous); > if (obj == NULL) { > - error_set(&err, QERR_DEVICE_NOT_FOUND, path); > + error_set(&err, ERROR_CLASS_DEVICE_NOT_FOUND, > + "Device '%s' not found", path); > } else { > if (ambiguous) { > monitor_printf(mon, "Warning: Path '%s' is ambiguous\n", path); > diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h > index 6468e40..2841344 100644 > --- a/include/qapi/qmp/qerror.h > +++ b/include/qapi/qmp/qerror.h > @@ -55,9 +55,6 @@ void qerror_report_err(Error *err); > #define QERR_DEVICE_NO_HOTPLUG \ > ERROR_CLASS_GENERIC_ERROR, "Device '%s' does not support hotplugging" > > -#define QERR_DEVICE_NOT_FOUND \ > - ERROR_CLASS_DEVICE_NOT_FOUND, "Device '%s' not found" > - > #define QERR_FD_NOT_FOUND \ > ERROR_CLASS_GENERIC_ERROR, "File descriptor named '%s' not found" > > diff --git a/net/net.c b/net/net.c > index 429012f..212762d 100644 > --- a/net/net.c > +++ b/net/net.c > @@ -1109,7 +1109,8 @@ void qmp_netdev_del(const char *id, Error **errp) > > nc = qemu_find_netdev(id); > if (!nc) { > - error_set(errp, QERR_DEVICE_NOT_FOUND, id); > + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, > + "Device '%s' not found", id); > return; > } > > @@ -1220,7 +1221,8 @@ void qmp_set_link(const char *name, bool up, Error **errp) > MAX_QUEUE_NUM); > > if (queues == 0) { > - error_set(errp, QERR_DEVICE_NOT_FOUND, name); > + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, > + "Device '%s' not found", name); > return; > } > nc = ncs[0]; > diff --git a/qdev-monitor.c b/qdev-monitor.c > index e1dd367..7bd7d25 100644 > --- a/qdev-monitor.c > +++ b/qdev-monitor.c > @@ -457,7 +457,8 @@ static BusState *qbus_find(const char *path, Error **errp) > pos += len; > dev = qbus_find_dev(bus, elem); > if (!dev) { > - error_set(errp, QERR_DEVICE_NOT_FOUND, elem); > + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, > + "Device '%s' not found", elem); > #if 0 /* conversion from qerror_report() to error_set() broke this: */ > if (!monitor_cur_is_qmp()) { > qbus_list_dev(bus); > @@ -793,7 +794,8 @@ void qmp_device_del(const char *id, Error **errp) > g_free(path); > > if (!obj) { > - error_set(errp, QERR_DEVICE_NOT_FOUND, id); > + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, > + "Device '%s' not found", id); > return; > } > > diff --git a/qmp.c b/qmp.c > index 60658b4..14f7536 100644 > --- a/qmp.c > +++ b/qmp.c > @@ -206,7 +206,8 @@ ObjectPropertyInfoList *qmp_qom_list(const char *path, Error **errp) > if (ambiguous) { > error_setg(errp, "Path '%s' is ambiguous", path); > } else { > - error_set(errp, QERR_DEVICE_NOT_FOUND, path); > + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, > + "Device '%s' not found", path); > } > return NULL; > } > @@ -236,7 +237,8 @@ int qmp_qom_set(Monitor *mon, const QDict *qdict, QObject **ret) > > obj = object_resolve_path(path, NULL); > if (!obj) { > - error_set(&local_err, QERR_DEVICE_NOT_FOUND, path); > + error_set(&local_err, ERROR_CLASS_DEVICE_NOT_FOUND, > + "Device '%s' not found", path); > goto out; > } > > @@ -261,7 +263,8 @@ int qmp_qom_get(Monitor *mon, const QDict *qdict, QObject **ret) > > obj = object_resolve_path(path, NULL); > if (!obj) { > - error_set(&local_err, QERR_DEVICE_NOT_FOUND, path); > + error_set(&local_err, ERROR_CLASS_DEVICE_NOT_FOUND, > + "Device '%s' not found", path); > goto out; > } > > @@ -518,7 +521,8 @@ DevicePropertyInfoList *qmp_device_list_properties(const char *typename, > > klass = object_class_by_name(typename); > if (klass == NULL) { > - error_set(errp, QERR_DEVICE_NOT_FOUND, typename); > + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, > + "Device '%s' not found", typename); > return NULL; > } > > diff --git a/qom/object.c b/qom/object.c > index 96abd34..62fa44e 100644 > --- a/qom/object.c > +++ b/qom/object.c > @@ -890,7 +890,8 @@ Object *object_property_get_link(Object *obj, const char *name, > if (str && *str) { > target = object_resolve_path(str, NULL); > if (!target) { > - error_set(errp, QERR_DEVICE_NOT_FOUND, str); > + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, > + "Device '%s' not found", str); > } > } > > @@ -1170,7 +1171,8 @@ static Object *object_resolve_link(Object *obj, const char *name, > if (target || ambiguous) { > error_set(errp, QERR_INVALID_PARAMETER_TYPE, name, target_type); > } else { > - error_set(errp, QERR_DEVICE_NOT_FOUND, path); > + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, > + "Device '%s' not found", path); > } > target = NULL; > } > diff --git a/ui/input.c b/ui/input.c > index eeeabe8..e96e1ea 100644 > --- a/ui/input.c > +++ b/ui/input.c > @@ -84,7 +84,8 @@ void qemu_input_handler_bind(QemuInputHandlerState *s, > > dev = qdev_find_recursive(sysbus_get_default(), device_id); > if (dev == NULL) { > - error_set(errp, QERR_DEVICE_NOT_FOUND, device_id); > + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, > + "Device '%s' not found", device_id); > return; > } >
On 06/15/2015 09:18 AM, Luiz Capitulino wrote: > On Sat, 13 Jun 2015 16:20:51 +0200 > Markus Armbruster <armbru@redhat.com> wrote: > >> Error classes other than ERROR_CLASS_GENERIC_ERROR should not be used >> in new code. Hiding them in QERR_ macros makes new uses hard to spot. >> Fortunately, there's just one such macro left. Eliminate it with this >> coccinelle semantic patch: >> >> @@ >> expression EP, E; >> @@ >> -error_set(EP, QERR_DEVICE_NOT_FOUND, E) >> +error_set(EP, ERROR_CLASS_DEVICE_NOT_FOUND, "Device '%s' not found", E) > > This is a bit minor, but I think I'd have created a new function instead, > say error_set_enodev(). This avoids all the duplication. But I'm not asking > you to change, as the patch is good and this can be done in the future if > we so want. In fact, in both patch 4 and 5, I thought a similar thing - the remaining QERR_ macros that contain a % embedded in them are a bit unusual (when reading error_setg(), you have to go look up the QERR_ macro to see how many arguments you should really be passing). If I understand correctly, one of the reasons we went with QERR_ macros embedding % was to ensure consistent error messages among multiple call sites, in part if we want to enable error message translations (but that's a bigger project anyways, which you have argued that we may not even want). Having helper functions for the more common error messages can both reduce confusion (no hidden %) and duplication (callers don't need to repeat the same string). But I likewise agree with Luiz that it can be deferred to the future if desired.
On 15 June 2015 at 16:18, Luiz Capitulino <lcapitulino@redhat.com> wrote: > On Sat, 13 Jun 2015 16:20:51 +0200 > Markus Armbruster <armbru@redhat.com> wrote: > >> Error classes other than ERROR_CLASS_GENERIC_ERROR should not be used >> in new code. Hiding them in QERR_ macros makes new uses hard to spot. >> Fortunately, there's just one such macro left. Eliminate it with this >> coccinelle semantic patch: >> >> @@ >> expression EP, E; >> @@ >> -error_set(EP, QERR_DEVICE_NOT_FOUND, E) >> +error_set(EP, ERROR_CLASS_DEVICE_NOT_FOUND, "Device '%s' not found", E) > > This is a bit minor, but I think I'd have created a new function instead, > say error_set_enodev(). This avoids all the duplication. But I'm not asking > you to change, as the patch is good and this can be done in the future if > we so want. The thing about that kind of generic set-an-error function is that it encourages people to use it rather than providing an error message that's more specific and helpful for the particular situation. That might not be a problem in this patch (I haven't read it), but I mention it because I have a patch onlist elsewhere which undoes a bit of "generic error based on an errno" in favour of being more specific about why something didn't work. thanks -- PMM
Eric Blake <eblake@redhat.com> writes: > On 06/13/2015 08:20 AM, Markus Armbruster wrote: >> Error classes other than ERROR_CLASS_GENERIC_ERROR should not be used >> in new code. Hiding them in QERR_ macros makes new uses hard to spot. >> Fortunately, there's just one such macro left. Eliminate it with this >> coccinelle semantic patch: >> >> @@ >> expression EP, E; >> @@ >> -error_set(EP, QERR_DEVICE_NOT_FOUND, E) >> +error_set(EP, ERROR_CLASS_DEVICE_NOT_FOUND, "Device '%s' not found", E) >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> backends/rng-egd.c | 3 ++- >> blockdev-nbd.c | 3 ++- >> blockdev.c | 33 ++++++++++++++++++++++----------- >> hmp.c | 6 ++++-- >> include/qapi/qmp/qerror.h | 3 --- >> net/net.c | 6 ++++-- >> qdev-monitor.c | 6 ++++-- >> qmp.c | 12 ++++++++---- >> qom/object.c | 6 ++++-- >> ui/input.c | 3 ++- >> 10 files changed, 52 insertions(+), 29 deletions(-) > > Plain transformation would be closer to a 1:1 insertion/deletion count. > The larger insertion count is due to reflowing long lines after the > transformation. Does coccinelle do that for you, or do you have to > touch things up manually? It does, and most of the time it's a relief. > But I'm okay with the result. > > Reviewed-by: Eric Blake <eblake@redhat.com> Thanks!
Peter Maydell <peter.maydell@linaro.org> writes: > On 15 June 2015 at 16:18, Luiz Capitulino <lcapitulino@redhat.com> wrote: >> On Sat, 13 Jun 2015 16:20:51 +0200 >> Markus Armbruster <armbru@redhat.com> wrote: >> >>> Error classes other than ERROR_CLASS_GENERIC_ERROR should not be used >>> in new code. Hiding them in QERR_ macros makes new uses hard to spot. >>> Fortunately, there's just one such macro left. Eliminate it with this >>> coccinelle semantic patch: >>> >>> @@ >>> expression EP, E; >>> @@ >>> -error_set(EP, QERR_DEVICE_NOT_FOUND, E) >>> +error_set(EP, ERROR_CLASS_DEVICE_NOT_FOUND, "Device '%s' not found", E) >> >> This is a bit minor, but I think I'd have created a new function instead, >> say error_set_enodev(). This avoids all the duplication. But I'm not asking >> you to change, as the patch is good and this can be done in the future if >> we so want. > > The thing about that kind of generic set-an-error function is that > it encourages people to use it rather than providing an error message > that's more specific and helpful for the particular situation. That > might not be a problem in this patch (I haven't read it), but I mention > it because I have a patch onlist elsewhere which undoes a bit of > "generic error based on an errno" in favour of being more specific > about why something didn't work. Observation seconded. When creating bad error messages is easier or looks cleaner than creating good ones, you'll invariably end up with tons of bad ones. Back when we still pursued the "rich" error objects mistake, we had "easier" on steroids: for a new error, you had to create a macro, maybe add an error class, edit a table, and then use the macro. Hard to blame anyone for "sod it, I'll just reuse an existing one." Using a common error helper when it doesn't quite fit is a case of "looks cleaner". It's only (marginally) cleaner when it fits. But having one for the cases where it fits creates temptation for cases where it doesn't quite fit.
Eric Blake <eblake@redhat.com> writes: > On 06/15/2015 09:18 AM, Luiz Capitulino wrote: >> On Sat, 13 Jun 2015 16:20:51 +0200 >> Markus Armbruster <armbru@redhat.com> wrote: >> >>> Error classes other than ERROR_CLASS_GENERIC_ERROR should not be used >>> in new code. Hiding them in QERR_ macros makes new uses hard to spot. >>> Fortunately, there's just one such macro left. Eliminate it with this >>> coccinelle semantic patch: >>> >>> @@ >>> expression EP, E; >>> @@ >>> -error_set(EP, QERR_DEVICE_NOT_FOUND, E) >>> +error_set(EP, ERROR_CLASS_DEVICE_NOT_FOUND, "Device '%s' not found", E) >> >> This is a bit minor, but I think I'd have created a new function instead, >> say error_set_enodev(). This avoids all the duplication. But I'm not asking >> you to change, as the patch is good and this can be done in the future if >> we so want. > > In fact, in both patch 4 and 5, I thought a similar thing - the > remaining QERR_ macros that contain a % embedded in them are a bit > unusual (when reading error_setg(), you have to go look up the QERR_ > macro to see how many arguments you should really be passing). If I > understand correctly, one of the reasons we went with QERR_ macros > embedding % was to ensure consistent error messages among multiple call > sites, in part if we want to enable error message translations (but > that's a bigger project anyways, which you have argued that we may not > even want). > > Having helper functions for the more common error messages can both > reduce confusion (no hidden %) and duplication (callers don't need to > repeat the same string). But I likewise agree with Luiz that it can be > deferred to the future if desired. My preferred solution is to have a helper function for the *action* rather than just its error. Not always practical.
diff --git a/backends/rng-egd.c b/backends/rng-egd.c index 2962795..849bd7a 100644 --- a/backends/rng-egd.c +++ b/backends/rng-egd.c @@ -147,7 +147,8 @@ static void rng_egd_opened(RngBackend *b, Error **errp) s->chr = qemu_chr_find(s->chr_name); if (s->chr == NULL) { - error_set(errp, QERR_DEVICE_NOT_FOUND, s->chr_name); + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, + "Device '%s' not found", s->chr_name); return; } diff --git a/blockdev-nbd.c b/blockdev-nbd.c index 0d9df47..128e810 100644 --- a/blockdev-nbd.c +++ b/blockdev-nbd.c @@ -91,7 +91,8 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable, blk = blk_by_name(device); if (!blk) { - error_set(errp, QERR_DEVICE_NOT_FOUND, device); + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, + "Device '%s' not found", device); return; } if (!blk_is_inserted(blk)) { diff --git a/blockdev.c b/blockdev.c index a88ea76..8dec0cc 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1102,7 +1102,8 @@ SnapshotInfo *qmp_blockdev_snapshot_delete_internal_sync(const char *device, blk = blk_by_name(device); if (!blk) { - error_set(errp, QERR_DEVICE_NOT_FOUND, device); + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, + "Device '%s' not found", device); return NULL; } bs = blk_bs(blk); @@ -1291,7 +1292,8 @@ static void internal_snapshot_prepare(BlkTransactionState *common, /* 2. check for validation */ blk = blk_by_name(device); if (!blk) { - error_set(errp, QERR_DEVICE_NOT_FOUND, device); + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, + "Device '%s' not found", device); return; } bs = blk_bs(blk); @@ -1571,7 +1573,8 @@ static void drive_backup_prepare(BlkTransactionState *common, Error **errp) blk = blk_by_name(backup->device); if (!blk) { - error_set(errp, QERR_DEVICE_NOT_FOUND, backup->device); + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, + "Device '%s' not found", backup->device); return; } bs = blk_bs(blk); @@ -1841,7 +1844,8 @@ void qmp_eject(const char *device, bool has_force, bool force, Error **errp) blk = blk_by_name(device); if (!blk) { - error_set(errp, QERR_DEVICE_NOT_FOUND, device); + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, + "Device '%s' not found", device); return; } @@ -1901,7 +1905,8 @@ void qmp_change_blockdev(const char *device, const char *filename, blk = blk_by_name(device); if (!blk) { - error_set(errp, QERR_DEVICE_NOT_FOUND, device); + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, + "Device '%s' not found", device); return; } bs = blk_bs(blk); @@ -1960,7 +1965,8 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd, blk = blk_by_name(device); if (!blk) { - error_set(errp, QERR_DEVICE_NOT_FOUND, device); + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, + "Device '%s' not found", device); return; } bs = blk_bs(blk); @@ -2275,7 +2281,8 @@ void qmp_block_stream(const char *device, blk = blk_by_name(device); if (!blk) { - error_set(errp, QERR_DEVICE_NOT_FOUND, device); + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, + "Device '%s' not found", device); return; } bs = blk_bs(blk); @@ -2349,7 +2356,8 @@ void qmp_block_commit(const char *device, * scenario in which all optional arguments are omitted. */ blk = blk_by_name(device); if (!blk) { - error_set(errp, QERR_DEVICE_NOT_FOUND, device); + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, + "Device '%s' not found", device); return; } bs = blk_bs(blk); @@ -2461,7 +2469,8 @@ void qmp_drive_backup(const char *device, const char *target, blk = blk_by_name(device); if (!blk) { - error_set(errp, QERR_DEVICE_NOT_FOUND, device); + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, + "Device '%s' not found", device); return; } bs = blk_bs(blk); @@ -2675,7 +2684,8 @@ void qmp_drive_mirror(const char *device, const char *target, blk = blk_by_name(device); if (!blk) { - error_set(errp, QERR_DEVICE_NOT_FOUND, device); + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, + "Device '%s' not found", device); return; } bs = blk_bs(blk); @@ -2941,7 +2951,8 @@ void qmp_change_backing_file(const char *device, blk = blk_by_name(device); if (!blk) { - error_set(errp, QERR_DEVICE_NOT_FOUND, device); + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, + "Device '%s' not found", device); return; } bs = blk_bs(blk); diff --git a/hmp.c b/hmp.c index 05e4927..6f63927 100644 --- a/hmp.c +++ b/hmp.c @@ -1863,7 +1863,8 @@ void hmp_qemu_io(Monitor *mon, const QDict *qdict) if (blk) { qemuio_command(blk, command); } else { - error_set(&err, QERR_DEVICE_NOT_FOUND, device); + error_set(&err, ERROR_CLASS_DEVICE_NOT_FOUND, + "Device '%s' not found", device); } hmp_handle_error(mon, &err); @@ -1991,7 +1992,8 @@ void hmp_qom_set(Monitor *mon, const QDict *qdict) obj = object_resolve_path(path, &ambiguous); if (obj == NULL) { - error_set(&err, QERR_DEVICE_NOT_FOUND, path); + error_set(&err, ERROR_CLASS_DEVICE_NOT_FOUND, + "Device '%s' not found", path); } else { if (ambiguous) { monitor_printf(mon, "Warning: Path '%s' is ambiguous\n", path); diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h index 6468e40..2841344 100644 --- a/include/qapi/qmp/qerror.h +++ b/include/qapi/qmp/qerror.h @@ -55,9 +55,6 @@ void qerror_report_err(Error *err); #define QERR_DEVICE_NO_HOTPLUG \ ERROR_CLASS_GENERIC_ERROR, "Device '%s' does not support hotplugging" -#define QERR_DEVICE_NOT_FOUND \ - ERROR_CLASS_DEVICE_NOT_FOUND, "Device '%s' not found" - #define QERR_FD_NOT_FOUND \ ERROR_CLASS_GENERIC_ERROR, "File descriptor named '%s' not found" diff --git a/net/net.c b/net/net.c index 429012f..212762d 100644 --- a/net/net.c +++ b/net/net.c @@ -1109,7 +1109,8 @@ void qmp_netdev_del(const char *id, Error **errp) nc = qemu_find_netdev(id); if (!nc) { - error_set(errp, QERR_DEVICE_NOT_FOUND, id); + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, + "Device '%s' not found", id); return; } @@ -1220,7 +1221,8 @@ void qmp_set_link(const char *name, bool up, Error **errp) MAX_QUEUE_NUM); if (queues == 0) { - error_set(errp, QERR_DEVICE_NOT_FOUND, name); + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, + "Device '%s' not found", name); return; } nc = ncs[0]; diff --git a/qdev-monitor.c b/qdev-monitor.c index e1dd367..7bd7d25 100644 --- a/qdev-monitor.c +++ b/qdev-monitor.c @@ -457,7 +457,8 @@ static BusState *qbus_find(const char *path, Error **errp) pos += len; dev = qbus_find_dev(bus, elem); if (!dev) { - error_set(errp, QERR_DEVICE_NOT_FOUND, elem); + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, + "Device '%s' not found", elem); #if 0 /* conversion from qerror_report() to error_set() broke this: */ if (!monitor_cur_is_qmp()) { qbus_list_dev(bus); @@ -793,7 +794,8 @@ void qmp_device_del(const char *id, Error **errp) g_free(path); if (!obj) { - error_set(errp, QERR_DEVICE_NOT_FOUND, id); + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, + "Device '%s' not found", id); return; } diff --git a/qmp.c b/qmp.c index 60658b4..14f7536 100644 --- a/qmp.c +++ b/qmp.c @@ -206,7 +206,8 @@ ObjectPropertyInfoList *qmp_qom_list(const char *path, Error **errp) if (ambiguous) { error_setg(errp, "Path '%s' is ambiguous", path); } else { - error_set(errp, QERR_DEVICE_NOT_FOUND, path); + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, + "Device '%s' not found", path); } return NULL; } @@ -236,7 +237,8 @@ int qmp_qom_set(Monitor *mon, const QDict *qdict, QObject **ret) obj = object_resolve_path(path, NULL); if (!obj) { - error_set(&local_err, QERR_DEVICE_NOT_FOUND, path); + error_set(&local_err, ERROR_CLASS_DEVICE_NOT_FOUND, + "Device '%s' not found", path); goto out; } @@ -261,7 +263,8 @@ int qmp_qom_get(Monitor *mon, const QDict *qdict, QObject **ret) obj = object_resolve_path(path, NULL); if (!obj) { - error_set(&local_err, QERR_DEVICE_NOT_FOUND, path); + error_set(&local_err, ERROR_CLASS_DEVICE_NOT_FOUND, + "Device '%s' not found", path); goto out; } @@ -518,7 +521,8 @@ DevicePropertyInfoList *qmp_device_list_properties(const char *typename, klass = object_class_by_name(typename); if (klass == NULL) { - error_set(errp, QERR_DEVICE_NOT_FOUND, typename); + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, + "Device '%s' not found", typename); return NULL; } diff --git a/qom/object.c b/qom/object.c index 96abd34..62fa44e 100644 --- a/qom/object.c +++ b/qom/object.c @@ -890,7 +890,8 @@ Object *object_property_get_link(Object *obj, const char *name, if (str && *str) { target = object_resolve_path(str, NULL); if (!target) { - error_set(errp, QERR_DEVICE_NOT_FOUND, str); + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, + "Device '%s' not found", str); } } @@ -1170,7 +1171,8 @@ static Object *object_resolve_link(Object *obj, const char *name, if (target || ambiguous) { error_set(errp, QERR_INVALID_PARAMETER_TYPE, name, target_type); } else { - error_set(errp, QERR_DEVICE_NOT_FOUND, path); + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, + "Device '%s' not found", path); } target = NULL; } diff --git a/ui/input.c b/ui/input.c index eeeabe8..e96e1ea 100644 --- a/ui/input.c +++ b/ui/input.c @@ -84,7 +84,8 @@ void qemu_input_handler_bind(QemuInputHandlerState *s, dev = qdev_find_recursive(sysbus_get_default(), device_id); if (dev == NULL) { - error_set(errp, QERR_DEVICE_NOT_FOUND, device_id); + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, + "Device '%s' not found", device_id); return; }
Error classes other than ERROR_CLASS_GENERIC_ERROR should not be used in new code. Hiding them in QERR_ macros makes new uses hard to spot. Fortunately, there's just one such macro left. Eliminate it with this coccinelle semantic patch: @@ expression EP, E; @@ -error_set(EP, QERR_DEVICE_NOT_FOUND, E) +error_set(EP, ERROR_CLASS_DEVICE_NOT_FOUND, "Device '%s' not found", E) Signed-off-by: Markus Armbruster <armbru@redhat.com> --- backends/rng-egd.c | 3 ++- blockdev-nbd.c | 3 ++- blockdev.c | 33 ++++++++++++++++++++++----------- hmp.c | 6 ++++-- include/qapi/qmp/qerror.h | 3 --- net/net.c | 6 ++++-- qdev-monitor.c | 6 ++++-- qmp.c | 12 ++++++++---- qom/object.c | 6 ++++-- ui/input.c | 3 ++- 10 files changed, 52 insertions(+), 29 deletions(-)