[Qemu-devel] [PATCH for-2.10 0/2] block: An empty filename counts as no filename

[Qemu-devel] [PATCH for-2.10 0/2] block: An empty filename counts as no filename

[Max Reitz]

See patch 1 for what is happening when you try "qemu-img info ''" and
for the fix.

(Patch 2 just adds a simple test.)


Max Reitz (2):
  block: An empty filename counts as no filename
  iotests/051: Add test for empty filename

 block.c                       | 2 +-
 tests/qemu-iotests/051        | 1 +
 tests/qemu-iotests/051.out    | 3 +++
 tests/qemu-iotests/051.pc.out | 3 +++
 4 files changed, 8 insertions(+), 1 deletion(-)

-- 
2.12.2

[Qemu-devel] [PATCH for-2.10 1/2] block: An empty filename counts as no filename

[Max Reitz]

Reproducer:
    $ ./qemu-img info ''
    qemu-img: ./block.c:1008: bdrv_open_driver: Assertion
        `!drv->bdrv_needs_filename || bs->filename[0]' failed.
    [1]    26105 abort (core dumped)  ./qemu-img info ''

This patch fixes this to be:
    $ ./qemu-img info ''
    qemu-img: Could not open '': The 'file' block driver requires a file
    name

Cc: qemu-stable <qemu-stable@nongnu.org>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 1fbbb8d606..46da908c93 100644
--- a/block.c
+++ b/block.c
@@ -1167,7 +1167,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file,
         filename = qdict_get_try_str(options, "filename");
     }
 
-    if (drv->bdrv_needs_filename && !filename) {
+    if (drv->bdrv_needs_filename && (!filename || !filename[0])) {
         error_setg(errp, "The '%s' block driver requires a file name",
                    drv->format_name);
         ret = -EINVAL;
-- 
2.12.2

Re: [Qemu-devel] [PATCH for-2.10 1/2] block: An empty filename counts as no filename

[Philippe Mathieu-Daudé]

Hi Max,

On 04/13/2017 01:06 PM, Max Reitz wrote:
> Reproducer:
>     $ ./qemu-img info ''
>     qemu-img: ./block.c:1008: bdrv_open_driver: Assertion
>         `!drv->bdrv_needs_filename || bs->filename[0]' failed.
>     [1]    26105 abort (core dumped)  ./qemu-img info ''
>
> This patch fixes this to be:
>     $ ./qemu-img info ''
>     qemu-img: Could not open '': The 'file' block driver requires a file
>     name
>
> Cc: qemu-stable <qemu-stable@nongnu.org>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block.c b/block.c
> index 1fbbb8d606..46da908c93 100644
> --- a/block.c
> +++ b/block.c
> @@ -1167,7 +1167,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file,
>          filename = qdict_get_try_str(options, "filename");
>      }
>
> -    if (drv->bdrv_needs_filename && !filename) {
> +    if (drv->bdrv_needs_filename && (!filename || !filename[0])) {

What do you think about adding an inline function in "qemu/option.h" 
like "is_valid_[option_]filename()" to avoid this bug template?

Anyway:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>          error_setg(errp, "The '%s' block driver requires a file name",
>                     drv->format_name);
>          ret = -EINVAL;
>

Re: [Qemu-devel] [PATCH for-2.10 1/2] block: An empty filename counts as no filename

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1874 bytes --]

On 17.04.2017 14:51, Philippe Mathieu-Daudé wrote:
> Hi Max,
> 
> On 04/13/2017 01:06 PM, Max Reitz wrote:
>> Reproducer:
>>     $ ./qemu-img info ''
>>     qemu-img: ./block.c:1008: bdrv_open_driver: Assertion
>>         `!drv->bdrv_needs_filename || bs->filename[0]' failed.
>>     [1]    26105 abort (core dumped)  ./qemu-img info ''
>>
>> This patch fixes this to be:
>>     $ ./qemu-img info ''
>>     qemu-img: Could not open '': The 'file' block driver requires a file
>>     name
>>
>> Cc: qemu-stable <qemu-stable@nongnu.org>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/block.c b/block.c
>> index 1fbbb8d606..46da908c93 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1167,7 +1167,7 @@ static int bdrv_open_common(BlockDriverState
>> *bs, BlockBackend *file,
>>          filename = qdict_get_try_str(options, "filename");
>>      }
>>
>> -    if (drv->bdrv_needs_filename && !filename) {
>> +    if (drv->bdrv_needs_filename && (!filename || !filename[0])) {
> 
> What do you think about adding an inline function in "qemu/option.h"
> like "is_valid_[option_]filename()" to avoid this bug template?

Well, I won't do it now but it's a good idea and if I hit this issue
again (or maybe if/when I realize how much code there is that uses
constructs like this...) I'll add it.

(Although it may actually be useful with a more common name, like
string_is_empty(). Unfortunately, there isn't anything like g_strlen0(),
apparently...)

> Anyway:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Thanks! (For your other reviews as well, of course.)

Max

> 
>>          error_setg(errp, "The '%s' block driver requires a file name",
>>                     drv->format_name);
>>          ret = -EINVAL;
>>



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

[Qemu-devel] [PATCH for-2.10 2/2] iotests/051: Add test for empty filename

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/051        | 1 +
 tests/qemu-iotests/051.out    | 3 +++
 tests/qemu-iotests/051.pc.out | 3 +++
 3 files changed, 7 insertions(+)

diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
index 630cb7a114..4fe676019b 100755
--- a/tests/qemu-iotests/051
+++ b/tests/qemu-iotests/051
@@ -231,6 +231,7 @@ echo === Leaving out required options ===
 echo
 
 run_qemu -drive driver=file
+run_qemu -drive driver=file,filename=
 run_qemu -drive driver=nbd
 run_qemu -drive driver=raw
 run_qemu -drive file.driver=file
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index 7524c62025..57bee086f0 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -221,6 +221,9 @@ QEMU X.Y.Z monitor - type 'help' for more information
 Testing: -drive driver=file
 QEMU_PROG: -drive driver=file: The 'file' block driver requires a file name
 
+Testing: -drive driver=file,filename=
+QEMU_PROG: -drive driver=file,filename=: The 'file' block driver requires a file name
+
 Testing: -drive driver=nbd
 QEMU_PROG: -drive driver=nbd: NBD server address missing
 
diff --git a/tests/qemu-iotests/051.pc.out b/tests/qemu-iotests/051.pc.out
index c6f4eef215..19426e5eda 100644
--- a/tests/qemu-iotests/051.pc.out
+++ b/tests/qemu-iotests/051.pc.out
@@ -319,6 +319,9 @@ QEMU X.Y.Z monitor - type 'help' for more information
 Testing: -drive driver=file
 QEMU_PROG: -drive driver=file: The 'file' block driver requires a file name
 
+Testing: -drive driver=file,filename=
+QEMU_PROG: -drive driver=file,filename=: The 'file' block driver requires a file name
+
 Testing: -drive driver=nbd
 QEMU_PROG: -drive driver=nbd: NBD server address missing
 
-- 
2.12.2

Re: [Qemu-devel] [Qemu-block] [PATCH for-2.10 0/2] block: An empty filename counts as no filename

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 590 bytes --]

On 04/13/2017 11:06 AM, Max Reitz wrote:
> See patch 1 for what is happening when you try "qemu-img info ''" and
> for the fix.
> 
> (Patch 2 just adds a simple test.)
> 
> 
> Max Reitz (2):
>   block: An empty filename counts as no filename
>   iotests/051: Add test for empty filename

Reviewed-by: Eric Blake <eblake@redhat.com>

Agree that this is not 2.9 material (but I see you cc'd qemu-stable for
1/2 going into 2.9.1, which is fine).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH for-2.10 0/2] block: An empty filename counts as no filename

[Fam Zheng]

On Thu, 04/13 18:06, Max Reitz wrote:
> See patch 1 for what is happening when you try "qemu-img info ''" and
> for the fix.
> 
> (Patch 2 just adds a simple test.)

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH for-2.10 0/2] block: An empty filename counts as no filename

[Kevin Wolf]

Am 13.04.2017 um 18:06 hat Max Reitz geschrieben:
> See patch 1 for what is happening when you try "qemu-img info ''" and
> for the fix.
> 
> (Patch 2 just adds a simple test.)

Thanks, applied to my block-next branch.

Kevin