[Qemu-devel] [PATCH 1/3] qemu-io: delete bs instead of leaking it

[Qemu-devel] [PATCH 1/3] qemu-io: delete bs instead of leaking it

[Stefan Hajnoczi]

Using bdrv_close() is not enough to free a BlockDriverState.  Since we
explicitly create it with bdrv_new(), use bdrv_delete() to close and
delete it.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 qemu-io.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/qemu-io.c b/qemu-io.c
index c45a413..5af887e 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -1582,7 +1582,7 @@ static const cmdinfo_t map_cmd = {
 
 static int close_f(int argc, char **argv)
 {
-    bdrv_close(bs);
+    bdrv_delete(bs);
     bs = NULL;
     return 0;
 }
@@ -1611,6 +1611,7 @@ static int openfile(char *name, int flags, int growable)
 
         if (bdrv_open(bs, name, flags, NULL) < 0) {
             fprintf(stderr, "%s: can't open device %s\n", progname, name);
+            bdrv_delete(bs);
             bs = NULL;
             return 1;
         }
@@ -1834,7 +1835,7 @@ int main(int argc, char **argv)
     qemu_aio_flush();
 
     if (bs) {
-        bdrv_close(bs);
+        bdrv_delete(bs);
     }
     return 0;
 }
-- 
1.7.7

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

[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);
-
     ret = refresh_total_sectors(bs, bs->total_sectors);
     if (ret < 0) {
         goto free_and_fail;
-- 
1.7.7

[Qemu-devel] [PATCH 3/3] block: reinitialize across bdrv_close()/bdrv_open()

[Stefan Hajnoczi]

Several BlockDriverState fields are not being reinitialized across
bdrv_close()/bdrv_open().  Make sure they are reset to their default
values.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 block.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 3207e99..b5e2aff 100644
--- a/block.c
+++ b/block.c
@@ -472,10 +472,13 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename,
     bs->total_sectors = 0;
     bs->encrypted = 0;
     bs->valid_key = 0;
+    bs->sg = 0;
     bs->open_flags = flags;
+    bs->growable = 0;
     bs->buffer_alignment = 512;
 
     pstrcpy(bs->filename, sizeof(bs->filename), filename);
+    bs->backing_file[0] = '\0';
 
     if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv)) {
         return -ENOTSUP;
@@ -484,8 +487,7 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename,
     bs->drv = drv;
     bs->opaque = g_malloc0(drv->instance_size);
 
-    if (flags & BDRV_O_CACHE_WB)
-        bs->enable_write_cache = 1;
+    bs->enable_write_cache = !!(flags & BDRV_O_CACHE_WB);
 
     /*
      * Clear flags that are internal to the block layer before opening the
-- 
1.7.7

Re: [Qemu-devel] [PATCH 2/3] block: set bs->read_only before .bdrv_open()

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH 2/3] block: set bs->read_only before .bdrv_open()

[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;
>>      }
>> +    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

Re: [Qemu-devel] [PATCH 2/3] block: set bs->read_only before .bdrv_open()

[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.

Stefan

Re: [Qemu-devel] [PATCH 2/3] block: set bs->read_only before .bdrv_open()

[Kevin Wolf]

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