Re: [Qemu-devel] [PATCH 1/3] blockdev: Add read-only option to change-blockdev

[Max Reitz]

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.