| Message ID | 1416487488-8423-2-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <mreitz@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? 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(...); } Otherwise, looks okay to me.
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 <mreitz@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.
On 11/26/2014 09:36 AM, Max Reitz wrote: >>> - 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. Oh, I read too fast; I'm missing the difference between '&err' and 'errp'. I think we generally use the name 'local_err' instead of plain 'err', so that it is more obvious when we are collecting into '&local_err' with a need to propagate, vs. reusing the caller's 'errp' directly because we aren't further checking things. Okay, your code is correct after all. The pre-existing confusion of 'err' instead of 'local_err' might be worth fixing if you have a reason for a respin, but is not itself a reason for me to withhold approval. Reviewed-by: Eric Blake <eblake@redhat.com>
diff --git a/blockdev.c b/blockdev.c index a52f205..4140a27 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1721,7 +1721,8 @@ static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename, } void qmp_change_blockdev(const char *device, const char *filename, - const char *format, Error **errp) + const char *format, + BlockdevChangeReadOnlyMode read_only, Error **errp) { BlockBackend *blk; BlockDriverState *bs; @@ -1754,10 +1755,44 @@ void qmp_change_blockdev(const char *device, const char *filename, goto out; } - bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR; + switch (read_only) { + case BLOCKDEV_CHANGE_READ_ONLY_MODE_RETAIN: + bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR; + break; + + case BLOCKDEV_CHANGE_READ_ONLY_MODE_RO: + bdrv_flags = 0; + break; + + case BLOCKDEV_CHANGE_READ_ONLY_MODE_RW: + bdrv_flags = BDRV_O_RDWR; + break; + + case BLOCKDEV_CHANGE_READ_ONLY_MODE_AUTO: + /* try RDWR first */ + bdrv_flags = BDRV_O_RDWR; + break; + + default: + abort(); + } + bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0; - 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); + } + } out: aio_context_release(aio_context); diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h index 09d1e30..14b4dfb 100644 --- a/include/sysemu/blockdev.h +++ b/include/sysemu/blockdev.h @@ -66,7 +66,8 @@ DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type); DriveInfo *add_init_drive(const char *opts); void qmp_change_blockdev(const char *device, const char *filename, - const char *format, Error **errp); + const char *format, + BlockdevChangeReadOnlyMode read_only, Error **errp); void do_commit(Monitor *mon, const QDict *qdict); int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data); #endif diff --git a/qapi-schema.json b/qapi-schema.json index d0926d9..441e001 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1543,6 +1543,26 @@ { 'command': 'change-vnc-password', 'data': {'password': 'str'} } ## +# @BlockdevChangeReadOnlyMode: +# +# Specifies the new read-only mode of a block device subject to the @change +# command. +# +# @retain: Retains the current read-only mode +# +# @ro: Makes the device read-only +# +# @rw: Makes the device writable +# +# @auto: If the file specified can be opened with write access, the block +# device will be writable; otherwise it will be read-only +# +# Since: 2.3 +## +{ 'enum': 'BlockdevChangeReadOnlyMode', + 'data': ['retain', 'ro', 'rw', 'auto'] } + +## # @change: # # This command is multiple commands multiplexed together. diff --git a/qmp.c b/qmp.c index 0b4f131..bd63cf4 100644 --- a/qmp.c +++ b/qmp.c @@ -402,7 +402,8 @@ void qmp_change(const char *device, const char *target, if (strcmp(device, "vnc") == 0) { qmp_change_vnc(target, has_arg, arg, errp); } else { - qmp_change_blockdev(device, target, arg, errp); + qmp_change_blockdev(device, target, arg, + BLOCKDEV_CHANGE_READ_ONLY_MODE_RETAIN, errp); } }
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 <mreitz@redhat.com> --- blockdev.c | 41 ++++++++++++++++++++++++++++++++++++++--- include/sysemu/blockdev.h | 3 ++- qapi-schema.json | 20 ++++++++++++++++++++ qmp.c | 3 ++- 4 files changed, 62 insertions(+), 5 deletions(-)