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 <kw...@redhat.com>
> To: Shahar Havivi <shah...@redhat.com>
> CC: qemu-devel@nongnu.org, Dor Laor <dl...@redhat.com>,
> Luiz Capitulino <lcapi...@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 <shah...@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 <shah...@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
