| Message ID | 1426269098-16043-1-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 03/13/2015 11:51 AM, Markus Armbruster wrote: > Error classes are a leftover from the days of "rich" error objects. > New code should always use ERROR_CLASS_GENERIC_ERROR. More precisely, new code should use ERROR_CLASS_GENERIC_ERROR unless there is a good reason where a caller knowing a different error class is likely to react differently as a result (for example, with 'drive-mirror', libvirt DOES rely on QERR_DEVICE_NOT_FOUND as a witness that the command supports an optional 'top' argument, vs. ERROR_CLASS_GENERIC_ERROR to state that 'top' was still mandatory). But I agree that this situation is one unlikely to be where libvirt cares what error class was used. > Commit e246211 > added a use of ERROR_CLASS_DEVICE_NOT_FOUND. Replace it. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > block/write-threshold.c | 2 +- > qapi/block-core.json | 4 ---- > 2 files changed, 1 insertion(+), 5 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com>
Eric Blake <eblake@redhat.com> writes: > On 03/13/2015 11:51 AM, Markus Armbruster wrote: >> Error classes are a leftover from the days of "rich" error objects. >> New code should always use ERROR_CLASS_GENERIC_ERROR. > > More precisely, new code should use ERROR_CLASS_GENERIC_ERROR unless > there is a good reason where a caller knowing a different error class is > likely to react differently as a result (for example, with > 'drive-mirror', libvirt DOES rely on QERR_DEVICE_NOT_FOUND as a witness > that the command supports an optional 'top' argument, vs. > ERROR_CLASS_GENERIC_ERROR to state that 'top' was still mandatory). In my experience, a caller / client wanting to figure out what exactly failed is often a sign of a function / command trying to do too much at once. Using error classes to introspect is trickery, committed out of desperation. We should offer proper introspection instead. That said, "always" is rather strong language. No rule without exceptions. If it bothers you, perhaps the word can be dropped on commit. > But I agree that this situation is one unlikely to be where libvirt > cares what error class was used. > >> Commit e246211 >> added a use of ERROR_CLASS_DEVICE_NOT_FOUND. Replace it. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> block/write-threshold.c | 2 +- >> qapi/block-core.json | 4 ---- >> 2 files changed, 1 insertion(+), 5 deletions(-) > > Reviewed-by: Eric Blake <eblake@redhat.com> Thanks!
Am 13.03.2015 um 18:51 hat Markus Armbruster geschrieben: > Error classes are a leftover from the days of "rich" error objects. > New code should always use ERROR_CLASS_GENERIC_ERROR. Commit e246211 > added a use of ERROR_CLASS_DEVICE_NOT_FOUND. Replace it. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> Thanks, applied to the block branch. Kevin
diff --git a/block/write-threshold.c b/block/write-threshold.c index c2cd517..a53c1f5 100644 --- a/block/write-threshold.c +++ b/block/write-threshold.c @@ -112,7 +112,7 @@ void qmp_block_set_write_threshold(const char *node_name, bs = bdrv_find_node(node_name); if (!bs) { - error_set(errp, QERR_DEVICE_NOT_FOUND, node_name); + error_setg(errp, "Device '%s' not found", node_name); return; } diff --git a/qapi/block-core.json b/qapi/block-core.json index 90586a5..42c8850 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1961,10 +1961,6 @@ # @write-threshold: configured threshold for the block device, bytes. # Use 0 to disable the threshold. # -# Returns: Nothing on success -# If @node name is not found on the block device graph, -# DeviceNotFound -# # Since: 2.3 ## { 'command': 'block-set-write-threshold',
Error classes are a leftover from the days of "rich" error objects. New code should always use ERROR_CLASS_GENERIC_ERROR. Commit e246211 added a use of ERROR_CLASS_DEVICE_NOT_FOUND. Replace it. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- block/write-threshold.c | 2 +- qapi/block-core.json | 4 ---- 2 files changed, 1 insertion(+), 5 deletions(-)