|
From: | Max Reitz |
Subject: | Re: [Qemu-devel] [PATCH 1/3] blockdev: Add read-only option to change-blockdev |
Date: | Wed, 26 Nov 2014 17:36:05 +0100 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 |
On 2014-11-26 at 17:24, Eric Blake wrote:
On 11/20/2014 05:44 AM, Max Reitz wrote:Add an option to qmp_change_blockdev() which allows changing the read-only status of the block device to be changed. Some drives do not have a inherently fixed read-only status; for instance, floppy disks can be set read-only or writable independently of the drive. Some users may find it useful to be able to therefore change the read-only status of a block device when changing the medium. Signed-off-by: Max Reitz <address@hidden> --- blockdev.c | 41 ++++++++++++++++++++++++++++++++++++++--- include/sysemu/blockdev.h | 3 ++- qapi-schema.json | 20 ++++++++++++++++++++ qmp.c | 3 ++- 4 files changed, 62 insertions(+), 5 deletions(-)- qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, NULL, errp);+ qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, NULL, &err); + + if (err) { + if (read_only == BLOCKDEV_CHANGE_READ_ONLY_MODE_AUTO) { + error_free(err); + err = NULL; + + /* RDWR did not work, try RO now */ + bdrv_flags &= ~BDRV_O_RDWR; + qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, NULL, errp); + } else { + error_propagate(errp, err); + }Umm, why are you propagating the error here manually, when it was previously propagated as part of the fall-through into the out: label?
Is it? I don't see any error_propagate() after that qmp_bdrv_open_encrypted_call()... And also, that call takes "errp" as a parameter, so having error_propagate() afterwards would be kind of strange.
The tree I'm looking at is master, commit 3ef4ebcc5c0360629bb05f49d9b961774247d6ca. In my block-next tree, which this patch is actually based against, commit 656ddfaafd67af2b9234567e51cd3418588b2ddd.
Particularly since the second open_encrypted call still relies on fallthrough for propagating the error? I think this should be simplified to: if (err && read_only == BLOCKDEV_CHANGE_READ_ONLY_MODE_AUTO) { error_free(err); err = NULL; /* RDWR did not work, try RO now */ bdrv_flags &= ~BDRV_O_RDWR; qmp_bdrv_open_encrypted(...); }
But then you're not propagating the error anymore (in case of !err || read_only != BLOCKDEV_CHANGE_READ_ONLY_MODE_AUTO). As I said, at least in my tree, there is no error_propagate(errp, err); after this block. I may add it, though.
(to be completely sure: http://git.qemu.org/?p=qemu.git;a=blob;f=blockdev.c;h=57910b82c7adc3ce59173afeeebcd37ff2a3dfd0;hb=3ef4ebcc5c0360629bb05f49d9b961774247d6ca#l1727 and https://github.com/XanClic/qemu/blob/block-next/blockdev.c#L1760)
Max
Otherwise, looks okay to me.
[Prev in Thread] | Current Thread | [Next in Thread] |