diff mbox

Wrong error message in block_passwd command

Message ID 20100305151222.GA21283@redhat.com
State New
Headers show

Commit Message

Shahar Havivi March 5, 2010, 3:12 p.m. UTC 
Monitor command 'block_passwd' reports a wrong error message when
drive is not encrypted

Signed-off-by: Shahar Havivi <shaharh@redhat.com>
---
 block.c   |    9 ++++++---
 monitor.c |    7 ++++++-
 qerror.c  |    4 ++++
 qerror.h  |    3 +++
 4 files changed, 19 insertions(+), 4 deletions(-)

Comments

Kevin Wolf March 5, 2010, 3:22 p.m. UTC | #1 
Am 05.03.2010 16:12, schrieb Shahar Havivi:
> Monitor command 'block_passwd' reports a wrong error message when
> drive is not encrypted
> 
> Signed-off-by: Shahar Havivi <shaharh@redhat.com>
> ---
>  block.c   |    9 ++++++---
>  monitor.c |    7 ++++++-
>  qerror.c  |    4 ++++
>  qerror.h  |    3 +++
>  4 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 31d1ba4..dd484fa 100644
> --- a/block.c
> +++ b/block.c
> @@ -1132,10 +1132,13 @@ int bdrv_set_key(BlockDriverState *bs, const char *key)
>          if (ret < 0)
>              return ret;
>          if (!bs->encrypted)
> -            return 0;
> +            return -EINVAL;
> +    }

I think this part is wrong actually. Sorry for not catching it when you
sent me the patch first.

The logic here is that it's not an error to set the password for a file
which isn't encrypted itself, but its backing file is. In this case the
key is only set for the backing file and we return success.

The other parts look good to me.

Kevin
diff mbox

Patch

diff --git a/block.c b/block.c
index 31d1ba4..dd484fa 100644
--- a/block.c
+++ b/block.c
@@ -1132,10 +1132,13 @@  int bdrv_set_key(BlockDriverState *bs, const char *key)
         if (ret < 0)
             return ret;
         if (!bs->encrypted)
-            return 0;
+            return -EINVAL;
+    }
+    if (!bs->encrypted) {
+        return -EINVAL;
+    } else if (!bs->drv || !bs->drv->bdrv_set_key) {
+        return -ENOMEDIUM;
     }
-    if (!bs->encrypted || !bs->drv || !bs->drv->bdrv_set_key)
-        return -1;
     ret = bs->drv->bdrv_set_key(bs, key);
     if (ret < 0) {
         bs->valid_key = 0;
diff --git a/monitor.c b/monitor.c
index 19470d1..30dcbbe 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1042,6 +1042,7 @@  static int do_block_set_passwd(Monitor *mon, const QDict *qdict,
                                 QObject **ret_data)
 {
     BlockDriverState *bs;
+    int err;
 
     bs = bdrv_find(qdict_get_str(qdict, "device"));
     if (!bs) {
@@ -1049,7 +1050,11 @@  static int do_block_set_passwd(Monitor *mon, const QDict *qdict,
         return -1;
     }
 
-    if (bdrv_set_key(bs, qdict_get_str(qdict, "password")) < 0) {
+    err = bdrv_set_key(bs, qdict_get_str(qdict, "password"));
+    if (err == -EINVAL) {
+        qemu_error_new(QERR_DEVICE_NOT_ENCRYPTED, bdrv_get_device_name(bs));
+        return -1;
+    } else if (err < 0) {
         qemu_error_new(QERR_INVALID_PASSWORD);
         return -1;
     }
diff --git a/qerror.c b/qerror.c
index 2f657f4..4e63a54 100644
--- a/qerror.c
+++ b/qerror.c
@@ -49,6 +49,10 @@  static const QErrorStringTable qerror_table[] = {
         .desc      = "The %(device) is encrypted",
     },
     {
+        .error_fmt = QERR_DEVICE_NOT_ENCRYPTED,
+        .desc      = "Device '%(device)' is not encrypted",
+    },
+    {
         .error_fmt = QERR_DEVICE_LOCKED,
         .desc      = "Device %(device) is locked",
     },
diff --git a/qerror.h b/qerror.h
index ee59615..b93fff6 100644
--- a/qerror.h
+++ b/qerror.h
@@ -46,6 +46,9 @@  QError *qobject_to_qerror(const QObject *obj);
 #define QERR_DEVICE_ENCRYPTED \
     "{ 'class': 'DeviceEncrypted', 'data': { 'device': %s } }"
 
+#define QERR_DEVICE_NOT_ENCRYPTED \
+    "{ 'class': 'DeviceNotEncrypted', 'data': { 'device': %s } }"
+
 #define QERR_DEVICE_LOCKED                                      \
     "{ 'class': 'DeviceLocked', 'data': { 'device': %s } }"