| Message ID | 20181012115532.12645-4-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: Add auto-read-only option | expand |
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?
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
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.
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; } }
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(-)