A few block drivers will set the BDS read_only flag from their
.bdrv_open() function. This means the bs->read_only flag could
be set after we enable copy_on_read, as the BDRV_O_COPY_ON_READ
flag check occurs prior to the call to bdrv->bdrv_open().
This adds an error return to bdrv_set_read_only(), and an error will be
return if we try to set the BDS to read_only while copy_on_read is
enabled.
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
block.c | 10 +++++++++-
block/bochs.c | 5 ++++-
block/cloop.c | 5 ++++-
block/dmg.c | 6 +++++-
block/rbd.c | 6 +++++-
block/vvfat.c | 15 ++++++++++++---
include/block/block.h | 2 +-
7 files changed, 40 insertions(+), 9 deletions(-)
diff --git a/block.c b/block.c
index 7b4c7ef..f60d5ea 100644
--- a/block.c
+++ b/block.c
@@ -192,9 +192,17 @@ void path_combine(char *dest, int dest_size,
}
}
-void bdrv_set_read_only(BlockDriverState *bs, bool read_only)
+int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
{
+ /* Do not set read_only if copy_on_read is enabled */
+ if (bs->copy_on_read && read_only) {
+ error_setg(errp, "Cannot set node '%s' to r/o while COW enabled",
+ bdrv_get_device_or_node_name(bs));
+ return -EINVAL;
+ }
+
bs->read_only = read_only;
+ return 0;
}
void bdrv_get_full_backing_filename_from_filename(const char *backed,
diff --git a/block/bochs.c b/block/bochs.c
index bdc2831..a759b6e 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -110,7 +110,10 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
return -EINVAL;
}
- bdrv_set_read_only(bs, true); /* no write support yet */
+ 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) {
diff --git a/block/cloop.c b/block/cloop.c
index 11f17c8..d6597fc 100644
--- a/block/cloop.c
+++ b/block/cloop.c
@@ -72,7 +72,10 @@ static int cloop_open(BlockDriverState *bs, QDict *options, int flags,
return -EINVAL;
}
- bdrv_set_read_only(bs, true);
+ 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);
diff --git a/block/dmg.c b/block/dmg.c
index 27ce4a6..900ae5a 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -419,8 +419,12 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
return -EINVAL;
}
+ ret = bdrv_set_read_only(bs, true, errp);
+ if (ret < 0) {
+ return ret;
+ }
+
block_module_load_one("dmg-bz2");
- bdrv_set_read_only(bs, true);
s->n_chunks = 0;
s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
diff --git a/block/rbd.c b/block/rbd.c
index 6ad2904..328e4a9 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -641,7 +641,11 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
goto failed_open;
}
- bdrv_set_read_only(bs, (s->snap != NULL));
+ r = bdrv_set_read_only(bs, (s->snap != NULL), &local_err);
+ if (r < 0) {
+ error_propagate(errp, local_err);
+ goto failed_open;
+ }
qemu_opts_del(opts);
return 0;
diff --git a/block/vvfat.c b/block/vvfat.c
index d4ce6d7..34a2854 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1156,8 +1156,6 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
s->current_cluster=0xffffffff;
- /* read only is the default for safety */
- bdrv_set_read_only(bs, true);
s->qcow = NULL;
s->qcow_filename = NULL;
s->fat2 = NULL;
@@ -1173,7 +1171,18 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
if (ret < 0) {
goto fail;
}
- bdrv_set_read_only(bs, false);
+ ret = bdrv_set_read_only(bs, false, &local_err);
+ if (ret < 0) {
+ error_propagate(errp, local_err);
+ goto fail;
+ }
+ } else {
+ /* read only is the default for safety */
+ ret = bdrv_set_read_only(bs, true, &local_err);
+ if (ret < 0) {
+ error_propagate(errp, local_err);
+ goto fail;
+ }
}
bs->total_sectors = cyls * heads * secs;
diff --git a/include/block/block.h b/include/block/block.h
index 06c9032..beb563a 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -426,7 +426,7 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
int64_t sector_num, int nb_sectors, int *pnum);
bool bdrv_is_read_only(BlockDriverState *bs);
-void bdrv_set_read_only(BlockDriverState *bs, bool read_only);
+int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
bool bdrv_is_sg(BlockDriverState *bs);
bool bdrv_is_inserted(BlockDriverState *bs);
int bdrv_media_changed(BlockDriverState *bs);
--
2.9.3
On 04/05/2017 02:28 PM, Jeff Cody wrote:
> A few block drivers will set the BDS read_only flag from their
> .bdrv_open() function. This means the bs->read_only flag could
> be set after we enable copy_on_read, as the BDRV_O_COPY_ON_READ
> flag check occurs prior to the call to bdrv->bdrv_open().
>
> This adds an error return to bdrv_set_read_only(), and an error will be
> return if we try to set the BDS to read_only while copy_on_read is
> enabled.
>
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
> block.c | 10 +++++++++-
> block/bochs.c | 5 ++++-
> block/cloop.c | 5 ++++-
> block/dmg.c | 6 +++++-
> block/rbd.c | 6 +++++-
> block/vvfat.c | 15 ++++++++++++---
> include/block/block.h | 2 +-
> 7 files changed, 40 insertions(+), 9 deletions(-)
>
> diff --git a/block.c b/block.c
> index 7b4c7ef..f60d5ea 100644
> --- a/block.c
> +++ b/block.c
> @@ -192,9 +192,17 @@ void path_combine(char *dest, int dest_size,
> }
> }
>
> -void bdrv_set_read_only(BlockDriverState *bs, bool read_only)
> +int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
> {
> + /* Do not set read_only if copy_on_read is enabled */
> + if (bs->copy_on_read && read_only) {
> + error_setg(errp, "Cannot set node '%s' to r/o while COW enabled",
COW?
> + bdrv_get_device_or_node_name(bs));
> + return -EINVAL;
> + }
> +
> bs->read_only = read_only;
> + return 0;
> }
>
> void bdrv_get_full_backing_filename_from_filename(const char *backed,
> diff --git a/block/bochs.c b/block/bochs.c
> index bdc2831..a759b6e 100644
> --- a/block/bochs.c
> +++ b/block/bochs.c
> @@ -110,7 +110,10 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
> return -EINVAL;
> }
>
> - bdrv_set_read_only(bs, true); /* no write support yet */
> + 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) {
> diff --git a/block/cloop.c b/block/cloop.c
> index 11f17c8..d6597fc 100644
> --- a/block/cloop.c
> +++ b/block/cloop.c
> @@ -72,7 +72,10 @@ static int cloop_open(BlockDriverState *bs, QDict *options, int flags,
> return -EINVAL;
> }
>
> - bdrv_set_read_only(bs, true);
> + 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);
> diff --git a/block/dmg.c b/block/dmg.c
> index 27ce4a6..900ae5a 100644
> --- a/block/dmg.c
> +++ b/block/dmg.c
> @@ -419,8 +419,12 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
> return -EINVAL;
> }
>
> + ret = bdrv_set_read_only(bs, true, errp);
> + if (ret < 0) {
> + return ret;
> + }
> +
> block_module_load_one("dmg-bz2");
> - bdrv_set_read_only(bs, true);
>
> s->n_chunks = 0;
> s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
> diff --git a/block/rbd.c b/block/rbd.c
> index 6ad2904..328e4a9 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -641,7 +641,11 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
> goto failed_open;
> }
>
> - bdrv_set_read_only(bs, (s->snap != NULL));
> + r = bdrv_set_read_only(bs, (s->snap != NULL), &local_err);
> + if (r < 0) {
> + error_propagate(errp, local_err);
> + goto failed_open;
> + }
>
> qemu_opts_del(opts);
> return 0;
> diff --git a/block/vvfat.c b/block/vvfat.c
> index d4ce6d7..34a2854 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -1156,8 +1156,6 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
>
> s->current_cluster=0xffffffff;
>
> - /* read only is the default for safety */
> - bdrv_set_read_only(bs, true);
> s->qcow = NULL;
> s->qcow_filename = NULL;
> s->fat2 = NULL;
> @@ -1173,7 +1171,18 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
> if (ret < 0) {
> goto fail;
> }
> - bdrv_set_read_only(bs, false);
> + ret = bdrv_set_read_only(bs, false, &local_err);
> + if (ret < 0) {
> + error_propagate(errp, local_err);
> + goto fail;
> + }
> + } else {
> + /* read only is the default for safety */
> + ret = bdrv_set_read_only(bs, true, &local_err);
> + if (ret < 0) {
> + error_propagate(errp, local_err);
> + goto fail;
> + }
> }
>
> bs->total_sectors = cyls * heads * secs;
> diff --git a/include/block/block.h b/include/block/block.h
> index 06c9032..beb563a 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -426,7 +426,7 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
> int64_t sector_num, int nb_sectors, int *pnum);
>
> bool bdrv_is_read_only(BlockDriverState *bs);
> -void bdrv_set_read_only(BlockDriverState *bs, bool read_only);
> +int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
> bool bdrv_is_sg(BlockDriverState *bs);
> bool bdrv_is_inserted(BlockDriverState *bs);
> int bdrv_media_changed(BlockDriverState *bs);
>
On Wed, Apr 05, 2017 at 03:16:38PM -0400, John Snow wrote:
>
>
> On 04/05/2017 02:28 PM, Jeff Cody wrote:
> > A few block drivers will set the BDS read_only flag from their
> > .bdrv_open() function. This means the bs->read_only flag could
> > be set after we enable copy_on_read, as the BDRV_O_COPY_ON_READ
> > flag check occurs prior to the call to bdrv->bdrv_open().
> >
> > This adds an error return to bdrv_set_read_only(), and an error will be
> > return if we try to set the BDS to read_only while copy_on_read is
> > enabled.
> >
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> > block.c | 10 +++++++++-
> > block/bochs.c | 5 ++++-
> > block/cloop.c | 5 ++++-
> > block/dmg.c | 6 +++++-
> > block/rbd.c | 6 +++++-
> > block/vvfat.c | 15 ++++++++++++---
> > include/block/block.h | 2 +-
> > 7 files changed, 40 insertions(+), 9 deletions(-)
> >
> > diff --git a/block.c b/block.c
> > index 7b4c7ef..f60d5ea 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -192,9 +192,17 @@ void path_combine(char *dest, int dest_size,
> > }
> > }
> >
> > -void bdrv_set_read_only(BlockDriverState *bs, bool read_only)
> > +int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
> > {
> > + /* Do not set read_only if copy_on_read is enabled */
> > + if (bs->copy_on_read && read_only) {
> > + error_setg(errp, "Cannot set node '%s' to r/o while COW enabled",
>
> COW?
>
Mooo! You are right, that should be COR (or better yet, I should just write
it out - copy on read).
> > + bdrv_get_device_or_node_name(bs));
> > + return -EINVAL;
> > + }
> > +
> > bs->read_only = read_only;
> > + return 0;
> > }
> >
> > void bdrv_get_full_backing_filename_from_filename(const char *backed,
> > diff --git a/block/bochs.c b/block/bochs.c
> > index bdc2831..a759b6e 100644
> > --- a/block/bochs.c
> > +++ b/block/bochs.c
> > @@ -110,7 +110,10 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
> > return -EINVAL;
> > }
> >
> > - bdrv_set_read_only(bs, true); /* no write support yet */
> > + 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) {
> > diff --git a/block/cloop.c b/block/cloop.c
> > index 11f17c8..d6597fc 100644
> > --- a/block/cloop.c
> > +++ b/block/cloop.c
> > @@ -72,7 +72,10 @@ static int cloop_open(BlockDriverState *bs, QDict *options, int flags,
> > return -EINVAL;
> > }
> >
> > - bdrv_set_read_only(bs, true);
> > + 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);
> > diff --git a/block/dmg.c b/block/dmg.c
> > index 27ce4a6..900ae5a 100644
> > --- a/block/dmg.c
> > +++ b/block/dmg.c
> > @@ -419,8 +419,12 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
> > return -EINVAL;
> > }
> >
> > + ret = bdrv_set_read_only(bs, true, errp);
> > + if (ret < 0) {
> > + return ret;
> > + }
> > +
> > block_module_load_one("dmg-bz2");
> > - bdrv_set_read_only(bs, true);
> >
> > s->n_chunks = 0;
> > s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
> > diff --git a/block/rbd.c b/block/rbd.c
> > index 6ad2904..328e4a9 100644
> > --- a/block/rbd.c
> > +++ b/block/rbd.c
> > @@ -641,7 +641,11 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
> > goto failed_open;
> > }
> >
> > - bdrv_set_read_only(bs, (s->snap != NULL));
> > + r = bdrv_set_read_only(bs, (s->snap != NULL), &local_err);
> > + if (r < 0) {
> > + error_propagate(errp, local_err);
> > + goto failed_open;
> > + }
> >
> > qemu_opts_del(opts);
> > return 0;
> > diff --git a/block/vvfat.c b/block/vvfat.c
> > index d4ce6d7..34a2854 100644
> > --- a/block/vvfat.c
> > +++ b/block/vvfat.c
> > @@ -1156,8 +1156,6 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
> >
> > s->current_cluster=0xffffffff;
> >
> > - /* read only is the default for safety */
> > - bdrv_set_read_only(bs, true);
> > s->qcow = NULL;
> > s->qcow_filename = NULL;
> > s->fat2 = NULL;
> > @@ -1173,7 +1171,18 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
> > if (ret < 0) {
> > goto fail;
> > }
> > - bdrv_set_read_only(bs, false);
> > + ret = bdrv_set_read_only(bs, false, &local_err);
> > + if (ret < 0) {
> > + error_propagate(errp, local_err);
> > + goto fail;
> > + }
> > + } else {
> > + /* read only is the default for safety */
> > + ret = bdrv_set_read_only(bs, true, &local_err);
> > + if (ret < 0) {
> > + error_propagate(errp, local_err);
> > + goto fail;
> > + }
> > }
> >
> > bs->total_sectors = cyls * heads * secs;
> > diff --git a/include/block/block.h b/include/block/block.h
> > index 06c9032..beb563a 100644
> > --- a/include/block/block.h
> > +++ b/include/block/block.h
> > @@ -426,7 +426,7 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
> > int64_t sector_num, int nb_sectors, int *pnum);
> >
> > bool bdrv_is_read_only(BlockDriverState *bs);
> > -void bdrv_set_read_only(BlockDriverState *bs, bool read_only);
> > +int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
> > bool bdrv_is_sg(BlockDriverState *bs);
> > bool bdrv_is_inserted(BlockDriverState *bs);
> > int bdrv_media_changed(BlockDriverState *bs);
> >
On Wed, Apr 05, 2017 at 02:28:44PM -0400, Jeff Cody wrote:
Minor comments but:
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> diff --git a/block.c b/block.c
> index 7b4c7ef..f60d5ea 100644
> --- a/block.c
> +++ b/block.c
> @@ -192,9 +192,17 @@ void path_combine(char *dest, int dest_size,
> }
> }
>
> -void bdrv_set_read_only(BlockDriverState *bs, bool read_only)
> +int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
> {
> + /* Do not set read_only if copy_on_read is enabled */
> + if (bs->copy_on_read && read_only) {
> + error_setg(errp, "Cannot set node '%s' to r/o while COW enabled",
Users might be puzzled by "COR". The -drive option is called
"copy-on-read" so spelling it out is clearer than using an acronym.
> @@ -1173,7 +1171,18 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
> if (ret < 0) {
> goto fail;
> }
> - bdrv_set_read_only(bs, false);
> + ret = bdrv_set_read_only(bs, false, &local_err);
> + if (ret < 0) {
> + error_propagate(errp, local_err);
> + goto fail;
> + }
read_only = false by default. There's no need to set it now that you've
moved the bdrv_set_read_only(bs, true) call.
On Wed, Apr 05, 2017 at 02:28:44PM -0400, Jeff Cody wrote:
> @@ -1173,7 +1171,18 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
> if (ret < 0) {
> goto fail;
> }
> - bdrv_set_read_only(bs, false);
> + ret = bdrv_set_read_only(bs, false, &local_err);
> + if (ret < 0) {
> + error_propagate(errp, local_err);
> + goto fail;
> + }
I realized later in the series why you are doing this.
The error code path introduces a resource leak: enable_write_target()
has already been called and isn't cleaned up by the fail label.
It would be cleaner to check that bs is writable before calling
enable_write_target().
Stefan
© 2016 - 2026 Red Hat, Inc.