Patchwork Re: [PATCH] Wrong error message in block_passwd command

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

Comments

Shahar Havivi - March 5, 2010, 3:47 p.m.
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(-)
Luiz Capitulino - March 5, 2010, 3:56 p.m.
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.
Markus Armbruster - March 5, 2010, 4:47 p.m.
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.
Luiz Capitulino - March 5, 2010, 8:41 p.m.
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.

Patch

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