[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 3/9] block: Require auto-read-only for existi
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH v3 3/9] block: Require auto-read-only for existing fallbacks |
Date: |
Thu, 18 Oct 2018 15:23:56 +0200 |
User-agent: |
Mutt/1.10.1 (2018-07-13) |
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
- [Qemu-devel] [PATCH v3 6/9] curl: Support auto-read-only option, (continued)
- [Qemu-devel] [PATCH v3 6/9] curl: Support auto-read-only option, Kevin Wolf, 2018/10/17
- [Qemu-devel] [PATCH v3 2/9] block: Add auto-read-only option, Kevin Wolf, 2018/10/17
- [Qemu-devel] [PATCH v3 9/9] block: Make auto-read-only=on default for -drive, Kevin Wolf, 2018/10/17
- [Qemu-devel] [PATCH v3 5/9] file-posix: Support auto-read-only option, Kevin Wolf, 2018/10/17
- [Qemu-devel] [PATCH v3 1/9] block: Update flags in bdrv_set_read_only(), Kevin Wolf, 2018/10/17
- [Qemu-devel] [PATCH v3 3/9] block: Require auto-read-only for existing fallbacks, Kevin Wolf, 2018/10/17
- [Qemu-devel] [PATCH v3 7/9] gluster: Support auto-read-only option, Kevin Wolf, 2018/10/17
- [Qemu-devel] [PATCH v3 8/9] iscsi: Support auto-read-only option, Kevin Wolf, 2018/10/17