diff mbox

block: Fix blockdev-backup not to use funky error class

Message ID 1426577774-21605-1-git-send-email-armbru@redhat.com
State New
Headers show

Commit Message

Markus Armbruster March 17, 2015, 7:36 a.m. UTC 
Error classes are a leftover from the days of "rich" error objects.
New code should always use ERROR_CLASS_GENERIC_ERROR.  Commit
b7b9d39..7c6a4ab added uses of ERROR_CLASS_DEVICE_NOT_FOUND.  Replace
them.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 blockdev.c             |  8 ++++----
 qapi/block-core.json   |  3 ---
 tests/qemu-iotests/055 | 11 +++++++----
 3 files changed, 11 insertions(+), 11 deletions(-)

Comments

Markus Armbruster March 17, 2015, 7:49 a.m. UTC | #1 
I should mention that I hunted down all new instances of funky error
classes since 2.2.  This is the last one.  The other two are:

[PATCH] vnc: Fix QMP change not to use funky error class
[PATCH] block: Fix block-set-write-threshold not to use funky error class
Kevin Wolf March 19, 2015, 11:29 a.m. UTC | #2 
Am 17.03.2015 um 08:36 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
> b7b9d39..7c6a4ab added uses of ERROR_CLASS_DEVICE_NOT_FOUND.  Replace
> them.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Thanks, applied to the block branch.

Eric, please speak up if libvirt needs the error code for any of these.

Kevin
Markus Armbruster March 19, 2015, 11:51 a.m. UTC | #3 
Kevin Wolf <kwolf@redhat.com> writes:

> Am 17.03.2015 um 08:36 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
>> b7b9d39..7c6a4ab added uses of ERROR_CLASS_DEVICE_NOT_FOUND.  Replace
>> them.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Thanks, applied to the block branch.
>
> Eric, please speak up if libvirt needs the error code for any of these.

I grepped through the libvirt source code (should've mentioned that),
but review from an expert is of course always welcome!
Eric Blake March 19, 2015, 2:22 p.m. UTC | #4 
On 03/19/2015 05:29 AM, Kevin Wolf wrote:
> Am 17.03.2015 um 08:36 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
>> b7b9d39..7c6a4ab added uses of ERROR_CLASS_DEVICE_NOT_FOUND.  Replace
>> them.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> 
> Thanks, applied to the block branch.
> 
> Eric, please speak up if libvirt needs the error code for any of these.

Libvirt is not using blockdev-backup yet, and so by implication does not
care about the error class.

Reviewed-by: Eric Blake <eblake@redhat.com>
diff mbox

Patch

diff --git a/blockdev.c b/blockdev.c
index b9c1c0c..0669fcb 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1566,13 +1566,13 @@  static void blockdev_backup_prepare(BlkTransactionState *common, Error **errp)
 
     bs = bdrv_find(backup->device);
     if (!bs) {
-        error_set(errp, QERR_DEVICE_NOT_FOUND, backup->device);
+        error_setg(errp, "Device '%s' not found", backup->device);
         return;
     }
 
     target = bdrv_find(backup->target);
     if (!target) {
-        error_set(errp, QERR_DEVICE_NOT_FOUND, backup->target);
+        error_setg(errp, "Device '%s' not found", backup->target);
         return;
     }
 
@@ -2402,7 +2402,7 @@  void qmp_blockdev_backup(const char *device, const char *target,
 
     bs = bdrv_find(device);
     if (!bs) {
-        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        error_setg(errp, "Device '%s' not found", device);
         return;
     }
 
@@ -2411,7 +2411,7 @@  void qmp_blockdev_backup(const char *device, const char *target,
 
     target_bs = bdrv_find(target);
     if (!target_bs) {
-        error_set(errp, QERR_DEVICE_NOT_FOUND, target);
+        error_setg(errp, "Device '%s' not found", target);
         goto out;
     }
 
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 42c8850..8311993 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -881,9 +881,6 @@ 
 #
 # For the arguments, see the documentation of BlockdevBackup.
 #
-# Returns: Nothing on success.
-#          If @device or @target is not a valid block device, DeviceNotFound.
-#
 # Since 2.3
 ##
 { 'command': 'blockdev-backup', 'data': 'BlockdevBackup' }
diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
index e81d4d0..017a609 100755
--- a/tests/qemu-iotests/055
+++ b/tests/qemu-iotests/055
@@ -126,7 +126,10 @@  class TestSingleDrive(iotests.QMPTestCase):
 
     def do_test_device_not_found(self, cmd, **args):
         result = self.vm.qmp(cmd, **args)
-        self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+        if cmd == 'drive-backup':
+            self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+        else:
+            self.assert_qmp(result, 'error/class', 'GenericError')
 
     def test_device_not_found(self):
         self.do_test_device_not_found('drive-backup', device='nonexistent',
@@ -364,7 +367,7 @@  class TestSingleTransaction(iotests.QMPTestCase):
                           'sync': 'full' },
             }
         ])
-        self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+        self.assert_qmp(result, 'error/class', 'GenericError')
 
         result = self.vm.qmp('transaction', actions=[{
                 'type': 'blockdev-backup',
@@ -373,7 +376,7 @@  class TestSingleTransaction(iotests.QMPTestCase):
                           'sync': 'full' },
             }
         ])
-        self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+        self.assert_qmp(result, 'error/class', 'GenericError')
 
         result = self.vm.qmp('transaction', actions=[{
                 'type': 'blockdev-backup',
@@ -382,7 +385,7 @@  class TestSingleTransaction(iotests.QMPTestCase):
                           'sync': 'full' },
             }
         ])
-        self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+        self.assert_qmp(result, 'error/class', 'GenericError')
 
     def test_target_is_source(self):
         result = self.vm.qmp('transaction', actions=[{