| Message ID | 1319709268-7435-2-git-send-email-stefanha@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
Am 27.10.2011 11:54, schrieb Stefan Hajnoczi: > Several block drivers set bs->read_only in .bdrv_open() but > block.c:bdrv_open_common() clobbers its value. Additionally, QED uses > bdrv_is_read_only() in .bdrv_open() to decide whether to perform > consistency checks. > > The correct ordering is to initialize bs->read_only from the open flags > before calling .bdrv_open(). This way block drivers can override it if > necessary and can use bdrv_is_read_only() in .bdrv_open(). > > Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> > --- > block.c | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/block.c b/block.c > index 70aab63..3207e99 100644 > --- a/block.c > +++ b/block.c > @@ -500,6 +500,8 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename, > open_flags |= BDRV_O_RDWR; > } Not directly related, but the context made me wonder when we're making a BlockkDriverState writeable unconditionally. This is the full context: /* * Snapshots should be writable. */ if (bs->is_temporary) { open_flags |= BDRV_O_RDWR; } Does anyone understand what the point of this is? If the user requested read-only, he certainly wants to have read-only, even if he specified -snapshot as well. > + bs->keep_read_only = bs->read_only = !(open_flags & BDRV_O_RDWR); > + > /* Open the image, either directly or using a protocol */ > if (drv->bdrv_file_open) { > ret = drv->bdrv_file_open(bs, filename, open_flags); > @@ -514,8 +516,6 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename, > goto free_and_fail; > } > > - bs->keep_read_only = bs->read_only = !(open_flags & BDRV_O_RDWR); > - The assignment was already at the new place before 4dca4b6. Not sure if there was any real reason for moving it, though. Kevin
On Thu, Oct 27, 2011 at 11:18 AM, Kevin Wolf <kwolf@redhat.com> wrote: > Am 27.10.2011 11:54, schrieb Stefan Hajnoczi: >> Several block drivers set bs->read_only in .bdrv_open() but >> block.c:bdrv_open_common() clobbers its value. Additionally, QED uses >> bdrv_is_read_only() in .bdrv_open() to decide whether to perform >> consistency checks. >> >> The correct ordering is to initialize bs->read_only from the open flags >> before calling .bdrv_open(). This way block drivers can override it if >> necessary and can use bdrv_is_read_only() in .bdrv_open(). >> >> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> >> --- >> block.c | 4 ++-- >> 1 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/block.c b/block.c >> index 70aab63..3207e99 100644 >> --- a/block.c >> +++ b/block.c >> @@ -500,6 +500,8 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename, >> open_flags |= BDRV_O_RDWR; >> } >> + bs->keep_read_only = bs->read_only = !(open_flags & BDRV_O_RDWR); >> + >> /* Open the image, either directly or using a protocol */ >> if (drv->bdrv_file_open) { >> ret = drv->bdrv_file_open(bs, filename, open_flags); >> @@ -514,8 +516,6 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename, >> goto free_and_fail; >> } >> >> - bs->keep_read_only = bs->read_only = !(open_flags & BDRV_O_RDWR); >> - > > The assignment was already at the new place before 4dca4b6. Not sure if > there was any real reason for moving it, though. Naphtali: any ideas why your commit needed to move bs->read_only assignment? Stefan
On Thu, Oct 27, 2011 at 11:18 AM, Kevin Wolf <kwolf@redhat.com> wrote: > Am 27.10.2011 11:54, schrieb Stefan Hajnoczi: >> Several block drivers set bs->read_only in .bdrv_open() but >> block.c:bdrv_open_common() clobbers its value. Additionally, QED uses >> bdrv_is_read_only() in .bdrv_open() to decide whether to perform >> consistency checks. >> >> The correct ordering is to initialize bs->read_only from the open flags >> before calling .bdrv_open(). This way block drivers can override it if >> necessary and can use bdrv_is_read_only() in .bdrv_open(). >> >> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> >> --- >> block.c | 4 ++-- >> 1 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/block.c b/block.c >> index 70aab63..3207e99 100644 >> --- a/block.c >> +++ b/block.c >> @@ -500,6 +500,8 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename, >> open_flags |= BDRV_O_RDWR; >> } > > Not directly related, but the context made me wonder when we're making a > BlockkDriverState writeable unconditionally. This is the full context: > > > /* > * Snapshots should be writable. > */ > if (bs->is_temporary) { > open_flags |= BDRV_O_RDWR; > } > > Does anyone understand what the point of this is? If the user requested > read-only, he certainly wants to have read-only, even if he specified > -snapshot as well. Perhaps this is an attempt to support -drive file=pristine.img,readonly=on,snapshot=on. The idea being that the user absolutely wants to keep pristine.img unmodified. But the nature of backing files means we should automatically get this. Stefan
Am 27.10.2011 12:45, schrieb Stefan Hajnoczi: > On Thu, Oct 27, 2011 at 11:18 AM, Kevin Wolf <kwolf@redhat.com> wrote: >> Am 27.10.2011 11:54, schrieb Stefan Hajnoczi: >>> Several block drivers set bs->read_only in .bdrv_open() but >>> block.c:bdrv_open_common() clobbers its value. Additionally, QED uses >>> bdrv_is_read_only() in .bdrv_open() to decide whether to perform >>> consistency checks. >>> >>> The correct ordering is to initialize bs->read_only from the open flags >>> before calling .bdrv_open(). This way block drivers can override it if >>> necessary and can use bdrv_is_read_only() in .bdrv_open(). >>> >>> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> >>> --- >>> block.c | 4 ++-- >>> 1 files changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/block.c b/block.c >>> index 70aab63..3207e99 100644 >>> --- a/block.c >>> +++ b/block.c >>> @@ -500,6 +500,8 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename, >>> open_flags |= BDRV_O_RDWR; >>> } >> >> Not directly related, but the context made me wonder when we're making a >> BlockkDriverState writeable unconditionally. This is the full context: >> >> >> /* >> * Snapshots should be writable. >> */ >> if (bs->is_temporary) { >> open_flags |= BDRV_O_RDWR; >> } >> >> Does anyone understand what the point of this is? If the user requested >> read-only, he certainly wants to have read-only, even if he specified >> -snapshot as well. > > Perhaps this is an attempt to support -drive > file=pristine.img,readonly=on,snapshot=on. The idea being that the > user absolutely wants to keep pristine.img unmodified. But the nature > of backing files means we should automatically get this. I would have said that it breaks this command line. It all depends on what your expectation of the semantics of these options is. Mine would be that the disk is presented read-only to the guest (and the snapshot is done but useless). Kevin
diff --git a/block.c b/block.c index 70aab63..3207e99 100644 --- a/block.c +++ b/block.c @@ -500,6 +500,8 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename, open_flags |= BDRV_O_RDWR; } + bs->keep_read_only = bs->read_only = !(open_flags & BDRV_O_RDWR); + /* Open the image, either directly or using a protocol */ if (drv->bdrv_file_open) { ret = drv->bdrv_file_open(bs, filename, open_flags); @@ -514,8 +516,6 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename, goto free_and_fail; } - bs->keep_read_only = bs->read_only = !(open_flags & BDRV_O_RDWR); - ret = refresh_total_sectors(bs, bs->total_sectors); if (ret < 0) { goto free_and_fail;
Several block drivers set bs->read_only in .bdrv_open() but block.c:bdrv_open_common() clobbers its value. Additionally, QED uses bdrv_is_read_only() in .bdrv_open() to decide whether to perform consistency checks. The correct ordering is to initialize bs->read_only from the open flags before calling .bdrv_open(). This way block drivers can override it if necessary and can use bdrv_is_read_only() in .bdrv_open(). Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> --- block.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)