[v2,3/8] block: Require auto-read-only for existing fallbacks

Message ID 20181012115532.12645-4-kwolf@redhat.com
State New
Headers show
Series
  • block: Add auto-read-only option
Related show

Commit Message

Kevin Wolf Oct. 12, 2018, 11:55 a.m.
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 <kwolf@redhat.com>
---
 include/block/block.h |  3 ++-
 block.c               | 37 +++++++++++++++++++++++--------------
 block/bochs.c         | 17 ++++++-----------
 block/cloop.c         | 16 +++++-----------
 block/dmg.c           | 16 +++++-----------
 block/rbd.c           | 14 ++++----------
 block/vvfat.c         | 12 +++---------
 7 files changed, 48 insertions(+), 67 deletions(-)

Comments

Eric Blake Oct. 12, 2018, 5:02 p.m. | #1
On 10/12/18 6:55 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 <kwolf@redhat.com>
> ---

> +++ b/block.c
> @@ -266,27 +266,36 @@ int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
>       return 0;
>   }
>   
> -/* TODO Remove (deprecated since 2.11)
> - * Block drivers are not supposed to automatically change bs->read_only.
> - * Instead, they should just check whether they can provide what the user
> - * explicitly requested and error out if read-write is requested, but they can
> - * only provide read-only access. */
> -int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
> +/*
> + * Called by a driver that can only provide a read-only image.
> + *
> + * Returns 0 if the node is already read-only or it could switch the node to
> + * read-only because BDRV_O_AUTO_RDONLY is set.
> + *
> + * Returns -EACCES if the node is read-write and BDRV_O_AUTO_RDONLY is not set.
> + * If @errmsg is not NULL, it is used as the error message for the Error
> + * object.

I like it.

Worth documenting the -EINVAL (copy-on-read prevents setting read-only) 
failure as well?  (The -EPERM failure of bdrv_can_set_read_only() is not 
reachable, since this new function never clears readonly).

> + */
> +int bdrv_apply_auto_read_only(BlockDriverState *bs, const char *errmsg,
> +                              Error **errp)
>   {
>       int ret = 0;
>   
> -    ret = bdrv_can_set_read_only(bs, read_only, false, errp);
> +    if (!(bs->open_flags & BDRV_O_RDWR)) {
> +        return 0;
> +    }
> +    if (!(bs->open_flags & BDRV_O_AUTO_RDONLY)) {
> +        error_setg(errp, "%s", errmsg ?: "Image is read-only");
> +        return -EACCES;
> +    }
> +
> +    ret = bdrv_can_set_read_only(bs, true, false, errp);
>       if (ret < 0) {
>           return ret;
>       }

Makes sense.

> +++ b/block/vvfat.c
> @@ -1262,16 +1262,10 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
>                          "Unable to set VVFAT to 'rw' when drive is read-only");
>               goto fail;
>           }
> -    } else  if (!bdrv_is_read_only(bs)) {
> -        error_report("Opening non-rw vvfat images 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.");
> -        /* read only is the default for safety */
> -        ret = bdrv_set_read_only(bs, true, &local_err);
> +    } else {
> +        ret = bdrv_apply_auto_read_only(bs, NULL, errp);
>           if (ret < 0) {
> -            error_propagate(errp, local_err);
> -            goto fail;
> +            return ret;

Don't you still need the goto fail, to avoid leaking opts?
Kevin Wolf Oct. 16, 2018, 2:12 p.m. | #2
Am 12.10.2018 um 19:02 hat Eric Blake geschrieben:
> On 10/12/18 6:55 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 <kwolf@redhat.com>
> > ---
> 
> > +++ b/block.c
> > @@ -266,27 +266,36 @@ int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
> >       return 0;
> >   }
> > -/* TODO Remove (deprecated since 2.11)
> > - * Block drivers are not supposed to automatically change bs->read_only.
> > - * Instead, they should just check whether they can provide what the user
> > - * explicitly requested and error out if read-write is requested, but they can
> > - * only provide read-only access. */
> > -int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
> > +/*
> > + * Called by a driver that can only provide a read-only image.
> > + *
> > + * Returns 0 if the node is already read-only or it could switch the node to
> > + * read-only because BDRV_O_AUTO_RDONLY is set.
> > + *
> > + * Returns -EACCES if the node is read-write and BDRV_O_AUTO_RDONLY is not set.
> > + * If @errmsg is not NULL, it is used as the error message for the Error
> > + * object.
> 
> I like it.
> 
> Worth documenting the -EINVAL (copy-on-read prevents setting read-only)
> failure as well?  (The -EPERM failure of bdrv_can_set_read_only() is not
> reachable, since this new function never clears readonly).

In fact, -EINVAL and the error string from bdrv_can_set_read_only() may
be confusing because the user didn't explicitly request a read-only
image. Maybe it would be better to just turn this case into -EACCES with
the same error message.

What do you think?

> > + */
> > +int bdrv_apply_auto_read_only(BlockDriverState *bs, const char *errmsg,
> > +                              Error **errp)
> >   {
> >       int ret = 0;
> > -    ret = bdrv_can_set_read_only(bs, read_only, false, errp);
> > +    if (!(bs->open_flags & BDRV_O_RDWR)) {
> > +        return 0;
> > +    }
> > +    if (!(bs->open_flags & BDRV_O_AUTO_RDONLY)) {
> > +        error_setg(errp, "%s", errmsg ?: "Image is read-only");
> > +        return -EACCES;
> > +    }
> > +
> > +    ret = bdrv_can_set_read_only(bs, true, false, errp);
> >       if (ret < 0) {
> >           return ret;
> >       }
> 
> Makes sense.
> 
> > +++ b/block/vvfat.c
> > @@ -1262,16 +1262,10 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
> >                          "Unable to set VVFAT to 'rw' when drive is read-only");
> >               goto fail;
> >           }
> > -    } else  if (!bdrv_is_read_only(bs)) {
> > -        error_report("Opening non-rw vvfat images 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.");
> > -        /* read only is the default for safety */
> > -        ret = bdrv_set_read_only(bs, true, &local_err);
> > +    } else {
> > +        ret = bdrv_apply_auto_read_only(bs, NULL, errp);
> >           if (ret < 0) {
> > -            error_propagate(errp, local_err);
> > -            goto fail;
> > +            return ret;
> 
> Don't you still need the goto fail, to avoid leaking opts?

Yes, I do. Thanks.

Kevin
Eric Blake Oct. 16, 2018, 6:51 p.m. | #3
On 10/16/18 9:12 AM, Kevin Wolf wrote:
> Am 12.10.2018 um 19:02 hat Eric Blake geschrieben:
>> On 10/12/18 6:55 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 <kwolf@redhat.com>
>>> ---
>>

>> Worth documenting the -EINVAL (copy-on-read prevents setting read-only)
>> failure as well?  (The -EPERM failure of bdrv_can_set_read_only() is not
>> reachable, since this new function never clears readonly).
> 
> In fact, -EINVAL and the error string from bdrv_can_set_read_only() may
> be confusing because the user didn't explicitly request a read-only
> image. Maybe it would be better to just turn this case into -EACCES with
> the same error message.
> 
> What do you think?

So, how would it trigger in practice? The user requests a copy-on-read 
action with the BDS as destination (thus the BDS must be writable, and 
can't be set to readonly); they omitted read-only (because they know 
they want copy-on-read); they supplied auto-read-only=true (because they 
are lazy and want to always use that flag if it is available); but the 
particular BDS they selected is not writable (whether read-only file 
system, read-only NBD server, etc).  In short, we can't grant them 
read-write to begin with, and can't gracefully fall back to read-only 
because it would violate their request for copy-on-read, so as long as 
we give them a sane error message about their request being impossible, 
we're good.  Yes, -EACCES sounds reasonable, if you want to code that in.

Patch

diff --git a/include/block/block.h b/include/block/block.h
index 580b3716c3..7f5453b45b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -438,7 +438,8 @@  int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
 bool bdrv_is_read_only(BlockDriverState *bs);
 int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
                            bool ignore_allow_rdw, Error **errp);
-int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
+int bdrv_apply_auto_read_only(BlockDriverState *bs, const char *errmsg,
+                              Error **errp);
 bool bdrv_is_writable(BlockDriverState *bs);
 bool bdrv_is_sg(BlockDriverState *bs);
 bool bdrv_is_inserted(BlockDriverState *bs);
diff --git a/block.c b/block.c
index f999393e28..631501bcae 100644
--- a/block.c
+++ b/block.c
@@ -266,27 +266,36 @@  int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
     return 0;
 }
 
-/* TODO Remove (deprecated since 2.11)
- * Block drivers are not supposed to automatically change bs->read_only.
- * Instead, they should just check whether they can provide what the user
- * explicitly requested and error out if read-write is requested, but they can
- * only provide read-only access. */
-int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
+/*
+ * Called by a driver that can only provide a read-only image.
+ *
+ * Returns 0 if the node is already read-only or it could switch the node to
+ * read-only because BDRV_O_AUTO_RDONLY is set.
+ *
+ * Returns -EACCES if the node is read-write and BDRV_O_AUTO_RDONLY is not set.
+ * If @errmsg is not NULL, it is used as the error message for the Error
+ * object.
+ */
+int bdrv_apply_auto_read_only(BlockDriverState *bs, const char *errmsg,
+                              Error **errp)
 {
     int ret = 0;
 
-    ret = bdrv_can_set_read_only(bs, read_only, false, errp);
+    if (!(bs->open_flags & BDRV_O_RDWR)) {
+        return 0;
+    }
+    if (!(bs->open_flags & BDRV_O_AUTO_RDONLY)) {
+        error_setg(errp, "%s", errmsg ?: "Image is read-only");
+        return -EACCES;
+    }
+
+    ret = bdrv_can_set_read_only(bs, true, false, errp);
     if (ret < 0) {
         return ret;
     }
 
-    bs->read_only = read_only;
-
-    if (read_only) {
-        bs->open_flags &= ~BDRV_O_RDWR;
-    } else {
-        bs->open_flags |= BDRV_O_RDWR;
-    }
+    bs->read_only = true;
+    bs->open_flags &= ~BDRV_O_RDWR;
 
     return 0;
 }
diff --git a/block/bochs.c b/block/bochs.c
index 50c630047b..22e7d44211 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -105,23 +105,18 @@  static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
     struct bochs_header bochs;
     int ret;
 
+    /* No write support yet */
+    ret = bdrv_apply_auto_read_only(bs, NULL, errp);
+    if (ret < 0) {
+        return ret;
+    }
+
     bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
                                false, errp);
     if (!bs->file) {
         return -EINVAL;
     }
 
-    if (!bdrv_is_read_only(bs)) {
-        error_report("Opening bochs images 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.");
-        ret = bdrv_set_read_only(bs, true, errp); /* no write support yet */
-        if (ret < 0) {
-            return ret;
-        }
-    }
-
     ret = bdrv_pread(bs->file, 0, &bochs, sizeof(bochs));
     if (ret < 0) {
         return ret;
diff --git a/block/cloop.c b/block/cloop.c
index 2be68987bd..df2b85f723 100644
--- a/block/cloop.c
+++ b/block/cloop.c
@@ -67,23 +67,17 @@  static int cloop_open(BlockDriverState *bs, QDict *options, int flags,
     uint32_t offsets_size, max_compressed_block_size = 1, i;
     int ret;
 
+    ret = bdrv_apply_auto_read_only(bs, NULL, errp);
+    if (ret < 0) {
+        return ret;
+    }
+
     bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
                                false, errp);
     if (!bs->file) {
         return -EINVAL;
     }
 
-    if (!bdrv_is_read_only(bs)) {
-        error_report("Opening cloop images 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.");
-        ret = bdrv_set_read_only(bs, true, errp);
-        if (ret < 0) {
-            return ret;
-        }
-    }
-
     /* read header */
     ret = bdrv_pread(bs->file, 128, &s->block_size, 4);
     if (ret < 0) {
diff --git a/block/dmg.c b/block/dmg.c
index c9b3c519c4..1d9283ba2f 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -413,23 +413,17 @@  static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
     int64_t offset;
     int ret;
 
+    ret = bdrv_apply_auto_read_only(bs, NULL, errp);
+    if (ret < 0) {
+        return ret;
+    }
+
     bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
                                false, errp);
     if (!bs->file) {
         return -EINVAL;
     }
 
-    if (!bdrv_is_read_only(bs)) {
-        error_report("Opening dmg images 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.");
-        ret = bdrv_set_read_only(bs, true, errp);
-        if (ret < 0) {
-            return ret;
-        }
-    }
-
     block_module_load_one("dmg-bz2");
 
     s->n_chunks = 0;
diff --git a/block/rbd.c b/block/rbd.c
index 014c68d629..ee0b4a6941 100644
--- a/block/rbd.c
+++ 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;
         }
     }
 
diff --git a/block/vvfat.c b/block/vvfat.c
index 98ba5e2bac..fd814c39c9 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1262,16 +1262,10 @@  static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
                        "Unable to set VVFAT to 'rw' when drive is read-only");
             goto fail;
         }
-    } else  if (!bdrv_is_read_only(bs)) {
-        error_report("Opening non-rw vvfat images 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.");
-        /* read only is the default for safety */
-        ret = bdrv_set_read_only(bs, true, &local_err);
+    } else {
+        ret = bdrv_apply_auto_read_only(bs, NULL, errp);
         if (ret < 0) {
-            error_propagate(errp, local_err);
-            goto fail;
+            return ret;
         }
     }