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 <mre...@redhat.com>
---
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.
