[for-2.10,1/2] block: An empty filename counts as no filename
diff mbox

Message ID 20170413160625.27719-2-mreitz@redhat.com
State New
Headers show

Commit Message

Max Reitz April 13, 2017, 4:06 p.m. UTC
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(-)

Comments

Philippe Mathieu-Daudé April 17, 2017, 12:51 p.m. UTC | #1
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;
>
Max Reitz April 19, 2017, 1:13 p.m. UTC | #2
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;
>>

Patch
diff mbox

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;