[Qemu-devel] [PATCH] Wrong error message in block_passwd command

[Qemu-devel] [PATCH] Wrong error message in block_passwd command

[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;
+    }
+    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 } }"
 
-- 
1.6.3.3

[Qemu-devel] Re: [PATCH] Wrong error message in block_passwd command

[Kevin Wolf]

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

[Qemu-devel] Re: [PATCH] Wrong error message in block_passwd command

[Shahar Havivi]

On Fri, Mar 05, 2010 at 04:22:55PM +0100, Kevin Wolf wrote:
> Date: Fri, 05 Mar 2010 16:22:55 +0100
> From: Kevin Wolf <kwolf@redhat.com>
> To: Shahar Havivi <shaharh@redhat.com>
> CC: qemu-devel@nongnu.org, Dor Laor <dlaor@redhat.com>,
> 	Luiz Capitulino <lcapitul@redhat.com>
> Subject: Re: [PATCH] Wrong error message in block_passwd command
> 
> 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
You right Kevin,
Revert returning error when backing file in bdrv_set_key.

Signed-off-by: Shahar Havivi <shaharh@redhat.com>

---
 block.c   |    7 +++++--
 monitor.c |    7 ++++++-
 qerror.c  |    4 ++++
 qerror.h  |    3 +++
 4 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index 31d1ba4..e891544 100644
--- a/block.c
+++ b/block.c
@@ -1134,8 +1134,11 @@ int bdrv_set_key(BlockDriverState *bs, const char *key)
         if (!bs->encrypted)
             return 0;
     }
-    if (!bs->encrypted || !bs->drv || !bs->drv->bdrv_set_key)
-        return -1;
+    if (!bs->encrypted) {
+        return -EINVAL;
+    } else if (!bs->drv || !bs->drv->bdrv_set_key) {
+        return -ENOMEDIUM;
+    }
     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 } }"
 
-- 
1.6.3.3

[Qemu-devel] Re: [PATCH] Wrong error message in block_passwd command

[Luiz Capitulino]

On Fri, 5 Mar 2010 17:47:33 +0200
Shahar Havivi <shaharh@redhat.com> wrote:

> On Fri, Mar 05, 2010 at 04:22:55PM +0100, Kevin Wolf wrote:
> > Date: Fri, 05 Mar 2010 16:22:55 +0100
> > From: Kevin Wolf <kwolf@redhat.com>
> > To: Shahar Havivi <shaharh@redhat.com>
> > CC: qemu-devel@nongnu.org, Dor Laor <dlaor@redhat.com>,
> > 	Luiz Capitulino <lcapitul@redhat.com>
> > Subject: Re: [PATCH] Wrong error message in block_passwd command
> > 
> > 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
> You right Kevin,
> Revert returning error when backing file in bdrv_set_key.

 Looks good to me too, but it's better to send it as new message, as
maintainers can miss patches submitted in the middle of thread.

Re: [Qemu-devel] Re: [PATCH] Wrong error message in block_passwd command

[Markus Armbruster]

Shahar Havivi <shaharh@redhat.com> writes:

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

Would you humor me and keep the error definitions sorted?  Thanks.


PS: Luiz has always put each new error in its own commit, and I followed
that practice.  Not sure we really care.

Re: [Qemu-devel] Re: [PATCH] Wrong error message in block_passwd command

[Luiz Capitulino]

On Fri, 05 Mar 2010 17:47:49 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Shahar Havivi <shaharh@redhat.com> writes:
> 
> > 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 } }"
> 
> Would you humor me and keep the error definitions sorted?  Thanks.

 A comment in the file will help.

> PS: Luiz has always put each new error in its own commit, and I followed
> that practice.  Not sure we really care.

 Well, turns out you were the only one to revert some of them :) So, I'm
ok with either way.