Patchwork Wrong error message in block_passwd command

login
register
mail settings
Submitter Shahar Havivi
Date March 5, 2010, 3:12 p.m.
Message ID <20100305151222.GA21283@redhat.com>
Download mbox | patch
Permalink /patch/47010/
State New
Headers show

Comments

Shahar Havivi - March 5, 2010, 3:12 p.m.
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(-)
Kevin Wolf - March 5, 2010, 3:22 p.m.
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

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 } }"