Re: [Qemu-devel] [PATCH 04/11] qerror: Eliminate QERR_DEVICE_NOT_FOUND
Peter Maydell writes: > On 15 June 2015 at 16:18, Luiz Capitulino wrote: >> On Sat, 13 Jun 2015 16:20:51 +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) >> >> 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.
Re: [Qemu-devel] [PATCH 04/11] qerror: Eliminate QERR_DEVICE_NOT_FOUND
Eric Blake writes: > On 06/15/2015 09:18 AM, Luiz Capitulino wrote: >> On Sat, 13 Jun 2015 16:20:51 +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) >> >> 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.
Re: [Qemu-devel] [PATCH 04/11] qerror: Eliminate QERR_DEVICE_NOT_FOUND
Eric Blake 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 >> --- >> 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 Thanks!
Re: [Qemu-devel] [PATCH 04/11] qerror: Eliminate QERR_DEVICE_NOT_FOUND
On 15 June 2015 at 16:18, Luiz Capitulino wrote: > On Sat, 13 Jun 2015 16:20:51 +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) > > 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
Re: [Qemu-devel] [PATCH 04/11] qerror: Eliminate QERR_DEVICE_NOT_FOUND
On 06/15/2015 09:18 AM, Luiz Capitulino wrote: > On Sat, 13 Jun 2015 16:20:51 +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) > > 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. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 04/11] qerror: Eliminate QERR_DEVICE_NOT_FOUND
On Sat, 13 Jun 2015 16:20:51 +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)
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
> ---
> 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 f
Re: [Qemu-devel] [PATCH 04/11] qerror: Eliminate QERR_DEVICE_NOT_FOUND
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 > --- > 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 pgpbe3j6zLgWV.pgp Description: PGP signature
Re: [Qemu-devel] [PATCH 04/11] qerror: Eliminate QERR_DEVICE_NOT_FOUND
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 > --- > 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 -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
[Qemu-devel] [PATCH 04/11] qerror: Eliminate QERR_DEVICE_NOT_FOUND
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
---
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_nam
