Re: [Qemu-block] [PATCH v3 3/9] block: Require auto-read-only for existing fallbacks

[Kevin Wolf]

Am 17.10.2018 um 20:53 hat Eric Blake geschrieben:
> On 10/17/18 11:41 AM, Kevin Wolf wrote:
> > Some block drivers have traditionally changed their node to read-only
> > mode without asking the user. This behaviour has been marked deprecated
> > since 2.11, expecting users to provide an explicit read-only=on option.
> > 
> > Now that we have auto-read-only=on, enable these drivers to make use of
> > the option.
> > 
> > This is the only use of bdrv_set_read_only(), so we can make it a bit
> > more specific and turn it into a bdrv_apply_auto_read_only() that is
> > more convenient for drivers to use.
> > 
> > Signed-off-by: Kevin Wolf <address@hidden>

> > +++ b/block/rbd.c
> > @@ -780,16 +780,10 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict 
> > *options, int flags,
> >       /* If we are using an rbd snapshot, we must be r/o, otherwise
> >        * leave as-is */
> >       if (s->snap != NULL) {
> > -        if (!bdrv_is_read_only(bs)) {
> > -            error_report("Opening rbd snapshots without an explicit "
> > -                         "read-only=on option is deprecated. Future 
> > versions "
> > -                         "will refuse to open the image instead of "
> > -                         "automatically marking the image read-only.");
> > -            r = bdrv_set_read_only(bs, true, &local_err);
> > -            if (r < 0) {
> > -                error_propagate(errp, local_err);
> > -                goto failed_open;
> > -            }
> > +        r = bdrv_apply_auto_read_only(bs, "rbd snapshots are read-only", 
> > errp);
> > +        if (r < 0) {
> > +            rbd_close(s->image);
> > +            goto failed_open;
> 
> That rbd_close() is an independent bugfix.  Should probably be split to a
> separate commit, or at a minimum called out in the commit message as
> intentional.

Okay, I'll split it.

> Actually, is it really needed to prevent a leak, or does the existing
> rados_shutdown() in failed_open already implicitly cover the actions of
> rbd_close()?

I don't know if rados_shutdown() would implicitly do that as well, but
the normal .bdrv_close implementation calls both, too, so I suppose the
safe option is to let the error path do the same.

Kevin