Patchwork [2/3] block: set bs->read_only before .bdrv_open()

login
register
mail settings
Submitter Stefan Hajnoczi
Date Oct. 27, 2011, 9:54 a.m.
Message ID <1319709268-7435-2-git-send-email-stefanha@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/122101/
State New
Headers show

Comments

Stefan Hajnoczi - Oct. 27, 2011, 9:54 a.m.
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(-)
Kevin Wolf - Oct. 27, 2011, 10:18 a.m.
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
Stefan Hajnoczi - Oct. 27, 2011, 10:41 a.m.
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
Stefan Hajnoczi - Oct. 27, 2011, 10:45 a.m.
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
Kevin Wolf - Oct. 27, 2011, 10:53 a.m.
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

Patch

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;