[Qemu-devel] [PATCH] block: fix backing file segfault

[Qemu-devel] [PATCH] block: fix backing file segfault

[Peter Feiner]

When a backing file is opened such that (1) a protocol is directly
used as the block driver and (2) the block driver has bdrv_file_open,
bdrv_open_backing_file segfaults. The problem arises because
bdrv_open_common returns without setting bd->backing_hd->file.

To effect (1), you seem to have to use the -F flag in qemu-img. There
are several block drivers that satisfy (2), such as "file" and "nbd".
Here are some concrete examples:

    #!/bin/bash

    echo Test file format
    ./qemu-img create -f file base.file 1m
    ./qemu-img create -f qcow2 -F file -o backing_file=base.file\
        file-overlay.qcow2
    ./qemu-img convert -O raw file-overlay.qcow2 file-convert.raw

    echo Test nbd format
    SOCK=$PWD/nbd.sock
    ./qemu-img create -f raw base.raw 1m
    ./qemu-nbd -t -k $SOCK base.raw &
    trap "kill $!" EXIT
    while ! test -e $SOCK; do sleep 1; done
    ./qemu-img create -f qcow2 -F nbd -o backing_file=nbd:unix:$SOCK\
        nbd-overlay.qcow2
    ./qemu-img convert -O raw nbd-overlay.qcow2 nbd-convert.raw

Without this patch, the two qemu-img convert commands segfault.

This is a regression that was introduced in v1.7 by
dbecebddfa4932d1c83915bcb9b5ba5984eb91be.

Signed-off-by: Peter Feiner <peter@gridcentric.ca>
---
 block.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 64e7d22..a4a172d 100644
--- a/block.c
+++ b/block.c
@@ -1016,8 +1016,9 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
         error_free(local_err);
         return ret;
     }
-    pstrcpy(bs->backing_file, sizeof(bs->backing_file),
-            bs->backing_hd->file->filename);
+    if (bs->backing_hd->file)
+        pstrcpy(bs->backing_file, sizeof(bs->backing_file),
+                bs->backing_hd->file->filename);
     return 0;
 }
 
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH] block: fix backing file segfault

[Fam Zheng]

Hi Peter,

Thank your for catching this.

On 2014年01月09日 03:43, Peter Feiner wrote:
> When a backing file is opened such that (1) a protocol is directly
> used as the block driver and (2) the block driver has bdrv_file_open,
> bdrv_open_backing_file segfaults. The problem arises because
> bdrv_open_common returns without setting bd->backing_hd->file.
>
> To effect (1), you seem to have to use the -F flag in qemu-img. There
> are several block drivers that satisfy (2), such as "file" and "nbd".
> Here are some concrete examples:
>
>      #!/bin/bash
>
>      echo Test file format
>      ./qemu-img create -f file base.file 1m
>      ./qemu-img create -f qcow2 -F file -o backing_file=base.file\
>          file-overlay.qcow2
>      ./qemu-img convert -O raw file-overlay.qcow2 file-convert.raw
>
>      echo Test nbd format
>      SOCK=$PWD/nbd.sock
>      ./qemu-img create -f raw base.raw 1m
>      ./qemu-nbd -t -k $SOCK base.raw &
>      trap "kill $!" EXIT
>      while ! test -e $SOCK; do sleep 1; done
>      ./qemu-img create -f qcow2 -F nbd -o backing_file=nbd:unix:$SOCK\
>          nbd-overlay.qcow2
>      ./qemu-img convert -O raw nbd-overlay.qcow2 nbd-convert.raw

It would be nice if you can add this as a test case under 
tests/qemu-iotests.

>
> Without this patch, the two qemu-img convert commands segfault.
>
> This is a regression that was introduced in v1.7 by
> dbecebddfa4932d1c83915bcb9b5ba5984eb91be.
>
> Signed-off-by: Peter Feiner <peter@gridcentric.ca>
> ---
>   block.c |    5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/block.c b/block.c
> index 64e7d22..a4a172d 100644
> --- a/block.c
> +++ b/block.c
> @@ -1016,8 +1016,9 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>           error_free(local_err);
>           return ret;
>       }
> -    pstrcpy(bs->backing_file, sizeof(bs->backing_file),
> -            bs->backing_hd->file->filename);
> +    if (bs->backing_hd->file)
> +        pstrcpy(bs->backing_file, sizeof(bs->backing_file),
> +                bs->backing_hd->file->filename);

Braces are mandatory for if statements (scripts/checkpatch.pl can check 
the coding style for you).

Thanks,
Fam

Re: [Qemu-devel] [PATCH] block: fix backing file segfault

[Kevin Wolf]

[ CCing Max, who was recently active in this area, for another opinion ]

Am 08.01.2014 um 20:43 hat Peter Feiner geschrieben:
> When a backing file is opened such that (1) a protocol is directly
> used as the block driver and (2) the block driver has bdrv_file_open,
> bdrv_open_backing_file segfaults. The problem arises because
> bdrv_open_common returns without setting bd->backing_hd->file.
> 
> To effect (1), you seem to have to use the -F flag in qemu-img. There
> are several block drivers that satisfy (2), such as "file" and "nbd".
> Here are some concrete examples:
> 
>     #!/bin/bash
> 
>     echo Test file format
>     ./qemu-img create -f file base.file 1m
>     ./qemu-img create -f qcow2 -F file -o backing_file=base.file\
>         file-overlay.qcow2
>     ./qemu-img convert -O raw file-overlay.qcow2 file-convert.raw
> 
>     echo Test nbd format
>     SOCK=$PWD/nbd.sock
>     ./qemu-img create -f raw base.raw 1m
>     ./qemu-nbd -t -k $SOCK base.raw &
>     trap "kill $!" EXIT
>     while ! test -e $SOCK; do sleep 1; done
>     ./qemu-img create -f qcow2 -F nbd -o backing_file=nbd:unix:$SOCK\
>         nbd-overlay.qcow2
>     ./qemu-img convert -O raw nbd-overlay.qcow2 nbd-convert.raw
> 
> Without this patch, the two qemu-img convert commands segfault.
> 
> This is a regression that was introduced in v1.7 by
> dbecebddfa4932d1c83915bcb9b5ba5984eb91be.
> 
> Signed-off-by: Peter Feiner <peter@gridcentric.ca>
> ---
>  block.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 64e7d22..a4a172d 100644
> --- a/block.c
> +++ b/block.c
> @@ -1016,8 +1016,9 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>          error_free(local_err);
>          return ret;
>      }
> -    pstrcpy(bs->backing_file, sizeof(bs->backing_file),
> -            bs->backing_hd->file->filename);
> +    if (bs->backing_hd->file)
> +        pstrcpy(bs->backing_file, sizeof(bs->backing_file),
> +                bs->backing_hd->file->filename);
>      return 0;
>  }

I think if there is no bs->backing_hd->file, we should get the filename
from bs->backing_hd->filename instead of leaving it empty.

In fact, can we always do that or does bs->backing_hd normally lack the
filename? If so, perhaps that is what we need to fix, so we can always
directly use bs->backing_hd->filename here.

Kevin

Re: [Qemu-devel] [PATCH] block: fix backing file segfault

[Peter Feiner]

On Thu, Jan 9, 2014 at 5:59 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> I think if there is no bs->backing_hd->file, we should get the filename
> from bs->backing_hd->filename instead of leaving it empty.
>
> In fact, can we always do that or does bs->backing_hd normally lack the
> filename? If so, perhaps that is what we need to fix, so we can always
> directly use bs->backing_hd->filename here.

In the NBD example, bs->backing_hd->filename is "".

I can take a stab at your proposed fix but I'm not very familiar with
this code. In particular, I don't know what bs->backing_file is
actually used for. The patch that introduced the pstrcpy mentions
hmp's info block command, but the output looks fine AFAICT:

% ./x86_64-softmmu/qemu-system-x86_64 -monitor stdio -nographic
file-overlay.qcow2
QEMU 1.7.50 monitor - type 'help' for more information
(qemu) info block
ide0-hd0: file-overlay.qcow2 (qcow2)
    Backing file:     base.file (chain depth: 1)
...

I'll take a look and report back.

On Thu, Jan 9, 2014 at 5:59 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> [ CCing Max, who was recently active in this area, for another opinion ]
>
> Am 08.01.2014 um 20:43 hat Peter Feiner geschrieben:
>> When a backing file is opened such that (1) a protocol is directly
>> used as the block driver and (2) the block driver has bdrv_file_open,
>> bdrv_open_backing_file segfaults. The problem arises because
>> bdrv_open_common returns without setting bd->backing_hd->file.
>>
>> To effect (1), you seem to have to use the -F flag in qemu-img. There
>> are several block drivers that satisfy (2), such as "file" and "nbd".
>> Here are some concrete examples:
>>
>>     #!/bin/bash
>>
>>     echo Test file format
>>     ./qemu-img create -f file base.file 1m
>>     ./qemu-img create -f qcow2 -F file -o backing_file=base.file\
>>         file-overlay.qcow2
>>     ./qemu-img convert -O raw file-overlay.qcow2 file-convert.raw
>>
>>     echo Test nbd format
>>     SOCK=$PWD/nbd.sock
>>     ./qemu-img create -f raw base.raw 1m
>>     ./qemu-nbd -t -k $SOCK base.raw &
>>     trap "kill $!" EXIT
>>     while ! test -e $SOCK; do sleep 1; done
>>     ./qemu-img create -f qcow2 -F nbd -o backing_file=nbd:unix:$SOCK\
>>         nbd-overlay.qcow2
>>     ./qemu-img convert -O raw nbd-overlay.qcow2 nbd-convert.raw
>>
>> Without this patch, the two qemu-img convert commands segfault.
>>
>> This is a regression that was introduced in v1.7 by
>> dbecebddfa4932d1c83915bcb9b5ba5984eb91be.
>>
>> Signed-off-by: Peter Feiner <peter@gridcentric.ca>
>> ---
>>  block.c |    5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 64e7d22..a4a172d 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1016,8 +1016,9 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>>          error_free(local_err);
>>          return ret;
>>      }
>> -    pstrcpy(bs->backing_file, sizeof(bs->backing_file),
>> -            bs->backing_hd->file->filename);
>> +    if (bs->backing_hd->file)
>> +        pstrcpy(bs->backing_file, sizeof(bs->backing_file),
>> +                bs->backing_hd->file->filename);
>>      return 0;
>>  }
>
> I think if there is no bs->backing_hd->file, we should get the filename
> from bs->backing_hd->filename instead of leaving it empty.
>
> In fact, can we always do that or does bs->backing_hd normally lack the
> filename? If so, perhaps that is what we need to fix, so we can always
> directly use bs->backing_hd->filename here.
>
> Kevin

Re: [Qemu-devel] [PATCH] block: fix backing file segfault

[Max Reitz]

On 09.01.2014 11:59, Kevin Wolf wrote:
> [ CCing Max, who was recently active in this area, for another opinion ]
>
> Am 08.01.2014 um 20:43 hat Peter Feiner geschrieben:
>> When a backing file is opened such that (1) a protocol is directly
>> used as the block driver and (2) the block driver has bdrv_file_open,
>> bdrv_open_backing_file segfaults. The problem arises because
>> bdrv_open_common returns without setting bd->backing_hd->file.
>>
>> To effect (1), you seem to have to use the -F flag in qemu-img. There
>> are several block drivers that satisfy (2), such as "file" and "nbd".
>> Here are some concrete examples:
>>
>>      #!/bin/bash
>>
>>      echo Test file format
>>      ./qemu-img create -f file base.file 1m
>>      ./qemu-img create -f qcow2 -F file -o backing_file=base.file\
>>          file-overlay.qcow2
>>      ./qemu-img convert -O raw file-overlay.qcow2 file-convert.raw
>>
>>      echo Test nbd format
>>      SOCK=$PWD/nbd.sock
>>      ./qemu-img create -f raw base.raw 1m
>>      ./qemu-nbd -t -k $SOCK base.raw &
>>      trap "kill $!" EXIT
>>      while ! test -e $SOCK; do sleep 1; done
>>      ./qemu-img create -f qcow2 -F nbd -o backing_file=nbd:unix:$SOCK\
>>          nbd-overlay.qcow2
>>      ./qemu-img convert -O raw nbd-overlay.qcow2 nbd-convert.raw
>>
>> Without this patch, the two qemu-img convert commands segfault.
>>
>> This is a regression that was introduced in v1.7 by
>> dbecebddfa4932d1c83915bcb9b5ba5984eb91be.
>>
>> Signed-off-by: Peter Feiner <peter@gridcentric.ca>
>> ---
>>   block.c |    5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 64e7d22..a4a172d 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1016,8 +1016,9 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>>           error_free(local_err);
>>           return ret;
>>       }
>> -    pstrcpy(bs->backing_file, sizeof(bs->backing_file),
>> -            bs->backing_hd->file->filename);
>> +    if (bs->backing_hd->file)
>> +        pstrcpy(bs->backing_file, sizeof(bs->backing_file),
>> +                bs->backing_hd->file->filename);
>>       return 0;
>>   }
> I think if there is no bs->backing_hd->file, we should get the filename
> from bs->backing_hd->filename instead of leaving it empty.
>
> In fact, can we always do that or does bs->backing_hd normally lack the
> filename? If so, perhaps that is what we need to fix, so we can always
> directly use bs->backing_hd->filename here.

bs->backing_hd->filename would be set by the bdrv_open_common() in 
bdrv_open(), the filename is read from file->filename (if file != NULL; 
in this case, that would be bs->backing_hd->file->filename) or from the 
configuration option "filename".

The latter configuration option is not used by bdrv_open_backing_file(), 
as far as I can see. However, bs->backing_hd->file->filename is exactly 
the field the old code uses, therefore, using bs->backing_hd->filename 
directly should not break anything.

However, the patch does something different: If file is NULL, it leaves 
bs->backing_file unchanged; whereas using bs->backing_hd->filename would 
in this case result in the value of the "filename" option. I think 
leaving bs->backing_file unchanged is probably better, unless it is "" 
and the "filename" option is set.

If we want bs->backing_hd->filename to always point to a valid filename, 
we'd probably have to copy to contents of bs->backing_file there at some 
point in time, if it is not valid. But this is exactly a point in code 
where bs->backing_file is updated, so there'd be no gain if we instead 
updated bs->backing_hd->filename if necessary and then copied that to 
bs->backing_file, as long as there is no other place in the code where 
bs->backing_hd->filename always has to be a valid filename.

Thus, I think the patch is okay, but I'd probably prefer "if 
(bs->backing_hd->filename[0]) pstrcpy(..., bs->backing_hd->filename);" - 
although that should not differ from the given patch, unless the 
"filename" option is set for the backing_hd.


Max

Re: [Qemu-devel] [PATCH] block: fix backing file segfault

[Kevin Wolf]

Am 10.01.2014 um 18:27 hat Max Reitz geschrieben:
> On 09.01.2014 11:59, Kevin Wolf wrote:
> >[ CCing Max, who was recently active in this area, for another opinion ]
> >
> >Am 08.01.2014 um 20:43 hat Peter Feiner geschrieben:
> >>When a backing file is opened such that (1) a protocol is directly
> >>used as the block driver and (2) the block driver has bdrv_file_open,
> >>bdrv_open_backing_file segfaults. The problem arises because
> >>bdrv_open_common returns without setting bd->backing_hd->file.
> >>
> >>To effect (1), you seem to have to use the -F flag in qemu-img. There
> >>are several block drivers that satisfy (2), such as "file" and "nbd".
> >>Here are some concrete examples:
> >>
> >>     #!/bin/bash
> >>
> >>     echo Test file format
> >>     ./qemu-img create -f file base.file 1m
> >>     ./qemu-img create -f qcow2 -F file -o backing_file=base.file\
> >>         file-overlay.qcow2
> >>     ./qemu-img convert -O raw file-overlay.qcow2 file-convert.raw
> >>
> >>     echo Test nbd format
> >>     SOCK=$PWD/nbd.sock
> >>     ./qemu-img create -f raw base.raw 1m
> >>     ./qemu-nbd -t -k $SOCK base.raw &
> >>     trap "kill $!" EXIT
> >>     while ! test -e $SOCK; do sleep 1; done
> >>     ./qemu-img create -f qcow2 -F nbd -o backing_file=nbd:unix:$SOCK\
> >>         nbd-overlay.qcow2
> >>     ./qemu-img convert -O raw nbd-overlay.qcow2 nbd-convert.raw
> >>
> >>Without this patch, the two qemu-img convert commands segfault.
> >>
> >>This is a regression that was introduced in v1.7 by
> >>dbecebddfa4932d1c83915bcb9b5ba5984eb91be.
> >>
> >>Signed-off-by: Peter Feiner <peter@gridcentric.ca>
> >>---
> >>  block.c |    5 +++--
> >>  1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/block.c b/block.c
> >>index 64e7d22..a4a172d 100644
> >>--- a/block.c
> >>+++ b/block.c
> >>@@ -1016,8 +1016,9 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
> >>          error_free(local_err);
> >>          return ret;
> >>      }
> >>-    pstrcpy(bs->backing_file, sizeof(bs->backing_file),
> >>-            bs->backing_hd->file->filename);
> >>+    if (bs->backing_hd->file)
> >>+        pstrcpy(bs->backing_file, sizeof(bs->backing_file),
> >>+                bs->backing_hd->file->filename);
> >>      return 0;
> >>  }
> >I think if there is no bs->backing_hd->file, we should get the filename
> >from bs->backing_hd->filename instead of leaving it empty.
> >
> >In fact, can we always do that or does bs->backing_hd normally lack the
> >filename? If so, perhaps that is what we need to fix, so we can always
> >directly use bs->backing_hd->filename here.
> 
> bs->backing_hd->filename would be set by the bdrv_open_common() in
> bdrv_open(), the filename is read from file->filename (if file !=
> NULL; in this case, that would be bs->backing_hd->file->filename) or
> from the configuration option "filename".
> 
> The latter configuration option is not used by
> bdrv_open_backing_file(), as far as I can see. However,
> bs->backing_hd->file->filename is exactly the field the old code
> uses, therefore, using bs->backing_hd->filename directly should not
> break anything.
> 
> However, the patch does something different: If file is NULL, it
> leaves bs->backing_file unchanged; whereas using
> bs->backing_hd->filename would in this case result in the value of
> the "filename" option. I think leaving bs->backing_file unchanged is
> probably better, unless it is "" and the "filename" option is set.
> 
> If we want bs->backing_hd->filename to always point to a valid
> filename, we'd probably have to copy to contents of bs->backing_file
> there at some point in time, if it is not valid. But this is exactly
> a point in code where bs->backing_file is updated, so there'd be no
> gain if we instead updated bs->backing_hd->filename if necessary and
> then copied that to bs->backing_file, as long as there is no other
> place in the code where bs->backing_hd->filename always has to be a
> valid filename.
> 
> Thus, I think the patch is okay, but I'd probably prefer "if
> (bs->backing_hd->filename[0]) pstrcpy(...,
> bs->backing_hd->filename);" - although that should not differ from
> the given patch, unless the "filename" option is set for the
> backing_hd.

Ok, if you're happy with it, I'll apply it. Can I put your Reviewed-by
there?

In the long run, we need to get rid of all this copying anyway. I'm
imagining a BlockDriver function that returns a file name to reproduce
the same setup, and a removal of bs->backing_file and bs->file_name.

For some drivers, the returned filename would be a URL or some other
string that that particular driver can parse.

While doing that, we might also consider a fake protocol that handles
filenames like 'json:{"driver":"qcow2","lazy-refcounts":"on",...}',
because for some drivers this might be the only thing that comes close
to a filename as it is a single string at least...

Kevin

Re: [Qemu-devel] [PATCH] block: fix backing file segfault

[Max Reitz]

On 10.01.2014 18:55, Kevin Wolf wrote:
> Am 10.01.2014 um 18:27 hat Max Reitz geschrieben:
>> On 09.01.2014 11:59, Kevin Wolf wrote:
>>> [ CCing Max, who was recently active in this area, for another opinion ]
>>>
>>> Am 08.01.2014 um 20:43 hat Peter Feiner geschrieben:
>>>> When a backing file is opened such that (1) a protocol is directly
>>>> used as the block driver and (2) the block driver has bdrv_file_open,
>>>> bdrv_open_backing_file segfaults. The problem arises because
>>>> bdrv_open_common returns without setting bd->backing_hd->file.
>>>>
>>>> To effect (1), you seem to have to use the -F flag in qemu-img. There
>>>> are several block drivers that satisfy (2), such as "file" and "nbd".
>>>> Here are some concrete examples:
>>>>
>>>>      #!/bin/bash
>>>>
>>>>      echo Test file format
>>>>      ./qemu-img create -f file base.file 1m
>>>>      ./qemu-img create -f qcow2 -F file -o backing_file=base.file\
>>>>          file-overlay.qcow2
>>>>      ./qemu-img convert -O raw file-overlay.qcow2 file-convert.raw
>>>>
>>>>      echo Test nbd format
>>>>      SOCK=$PWD/nbd.sock
>>>>      ./qemu-img create -f raw base.raw 1m
>>>>      ./qemu-nbd -t -k $SOCK base.raw &
>>>>      trap "kill $!" EXIT
>>>>      while ! test -e $SOCK; do sleep 1; done
>>>>      ./qemu-img create -f qcow2 -F nbd -o backing_file=nbd:unix:$SOCK\
>>>>          nbd-overlay.qcow2
>>>>      ./qemu-img convert -O raw nbd-overlay.qcow2 nbd-convert.raw
>>>>
>>>> Without this patch, the two qemu-img convert commands segfault.
>>>>
>>>> This is a regression that was introduced in v1.7 by
>>>> dbecebddfa4932d1c83915bcb9b5ba5984eb91be.
>>>>
>>>> Signed-off-by: Peter Feiner <peter@gridcentric.ca>
>>>> ---
>>>>   block.c |    5 +++--
>>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/block.c b/block.c
>>>> index 64e7d22..a4a172d 100644
>>>> --- a/block.c
>>>> +++ b/block.c
>>>> @@ -1016,8 +1016,9 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>>>>           error_free(local_err);
>>>>           return ret;
>>>>       }
>>>> -    pstrcpy(bs->backing_file, sizeof(bs->backing_file),
>>>> -            bs->backing_hd->file->filename);
>>>> +    if (bs->backing_hd->file)
>>>> +        pstrcpy(bs->backing_file, sizeof(bs->backing_file),
>>>> +                bs->backing_hd->file->filename);
>>>>       return 0;
>>>>   }
>>> I think if there is no bs->backing_hd->file, we should get the filename
>> >from bs->backing_hd->filename instead of leaving it empty.
>>> In fact, can we always do that or does bs->backing_hd normally lack the
>>> filename? If so, perhaps that is what we need to fix, so we can always
>>> directly use bs->backing_hd->filename here.
>> bs->backing_hd->filename would be set by the bdrv_open_common() in
>> bdrv_open(), the filename is read from file->filename (if file !=
>> NULL; in this case, that would be bs->backing_hd->file->filename) or
>> from the configuration option "filename".
>>
>> The latter configuration option is not used by
>> bdrv_open_backing_file(), as far as I can see. However,
>> bs->backing_hd->file->filename is exactly the field the old code
>> uses, therefore, using bs->backing_hd->filename directly should not
>> break anything.
>>
>> However, the patch does something different: If file is NULL, it
>> leaves bs->backing_file unchanged; whereas using
>> bs->backing_hd->filename would in this case result in the value of
>> the "filename" option. I think leaving bs->backing_file unchanged is
>> probably better, unless it is "" and the "filename" option is set.
>>
>> If we want bs->backing_hd->filename to always point to a valid
>> filename, we'd probably have to copy to contents of bs->backing_file
>> there at some point in time, if it is not valid. But this is exactly
>> a point in code where bs->backing_file is updated, so there'd be no
>> gain if we instead updated bs->backing_hd->filename if necessary and
>> then copied that to bs->backing_file, as long as there is no other
>> place in the code where bs->backing_hd->filename always has to be a
>> valid filename.
>>
>> Thus, I think the patch is okay, but I'd probably prefer "if
>> (bs->backing_hd->filename[0]) pstrcpy(...,
>> bs->backing_hd->filename);" - although that should not differ from
>> the given patch, unless the "filename" option is set for the
>> backing_hd.
> Ok, if you're happy with it, I'll apply it. Can I put your Reviewed-by
> there?

Yes, feel free to.

> In the long run, we need to get rid of all this copying anyway. I'm
> imagining a BlockDriver function that returns a file name to reproduce
> the same setup, and a removal of bs->backing_file and bs->file_name.
>
> For some drivers, the returned filename would be a URL or some other
> string that that particular driver can parse.
>
> While doing that, we might also consider a fake protocol that handles
> filenames like 'json:{"driver":"qcow2","lazy-refcounts":"on",...}',
> because for some drivers this might be the only thing that comes close
> to a filename as it is a single string at least...

Urgh. *g*

I'm not sure if we should force every BDS to have a clearly defining 
file name. If there are options, which completely change the behavior of 
the block driver (I wouldn't consider lazy-refcounts one of them since 
it doesn't change the contents of the block device), I'd rather return 
NULL when asked for a file name. But then again, maybe an ugly filename 
is better than none at all…

In general, I'd prefer abandoning filenames* (especially protocol 
filenames) altogether. The set of options with which to recreate the 
same BDS is already available.

Max


*Of course, we need filenames for, well, opening files, but I'm 
referring to have an explicit string "filename" in addition to the 
option dicts (nearly) everywhere. And as far as I see it, we don't 
really want the user to specify a filename outside of the options 
anymore anyway, therefore we should probably not encourage him/her to do 
so by introducing filename abominations which may contain all options. ;-)

Re: [Qemu-devel] [PATCH] block: fix backing file segfault

[Kevin Wolf]

Am 10.01.2014 um 19:05 hat Max Reitz geschrieben:
> On 10.01.2014 18:55, Kevin Wolf wrote:
> >Ok, if you're happy with it, I'll apply it. Can I put your Reviewed-by
> >there?
> 
> Yes, feel free to.

Thanks, applied to the block branch.

Peter, no need for a second version of the patch then. :-)

> >In the long run, we need to get rid of all this copying anyway. I'm
> >imagining a BlockDriver function that returns a file name to reproduce
> >the same setup, and a removal of bs->backing_file and bs->file_name.
> >
> >For some drivers, the returned filename would be a URL or some other
> >string that that particular driver can parse.
> >
> >While doing that, we might also consider a fake protocol that handles
> >filenames like 'json:{"driver":"qcow2","lazy-refcounts":"on",...}',
> >because for some drivers this might be the only thing that comes close
> >to a filename as it is a single string at least...
> 
> Urgh. *g*
> 
> I'm not sure if we should force every BDS to have a clearly defining
> file name. If there are options, which completely change the
> behavior of the block driver (I wouldn't consider lazy-refcounts one
> of them since it doesn't change the contents of the block device),
> I'd rather return NULL when asked for a file name. But then again,
> maybe an ugly filename is better than none at all…

We need filenames for backing file relationships. For example, when you
take a live snapshot, we need to reference the old image. If you don't
use the filename, but driver-specific options, I believe this fails
today.

You might also want to set some options for the backing file in images
created with qemu-img.

The alternative would be to extend qcow2 to have something more complex
than a string to describe backing files. However, this would mean that
qcow2 is the only possible format for live snapshots.

> In general, I'd prefer abandoning filenames* (especially protocol
> filenames) altogether. The set of options with which to recreate the
> same BDS is already available.
> 
> Max
> 
> *Of course, we need filenames for, well, opening files, but I'm
> referring to have an explicit string "filename" in addition to the
> option dicts (nearly) everywhere.

Agreed. The reason why filenames are still passed separately and not
converted to a file.filename QDict entry is the convenience magic that
they enable (at least protocol names) and that file.filename doesn't
have in order to have less special cases with blockdev-add.

So what you'd need to do is to parse the protocol names in the top-level
function bdrv_open() and convert them into the right QDict entries.
Perhaps this is also a better place for the .bdrv_parse_filename()
calls. And then you could call a new bdrv_file_open() that doesn't take
a separate filename argument any more.

Kevin

Re: [Qemu-devel] [PATCH] block: fix backing file segfault

[Max Reitz]

On 10.01.2014 19:26, Kevin Wolf wrote:
> Am 10.01.2014 um 19:05 hat Max Reitz geschrieben:
>> On 10.01.2014 18:55, Kevin Wolf wrote:
>>> Ok, if you're happy with it, I'll apply it. Can I put your Reviewed-by
>>> there?
>> Yes, feel free to.
> Thanks, applied to the block branch.
>
> Peter, no need for a second version of the patch then. :-)
>
>>> In the long run, we need to get rid of all this copying anyway. I'm
>>> imagining a BlockDriver function that returns a file name to reproduce
>>> the same setup, and a removal of bs->backing_file and bs->file_name.
>>>
>>> For some drivers, the returned filename would be a URL or some other
>>> string that that particular driver can parse.
>>>
>>> While doing that, we might also consider a fake protocol that handles
>>> filenames like 'json:{"driver":"qcow2","lazy-refcounts":"on",...}',
>>> because for some drivers this might be the only thing that comes close
>>> to a filename as it is a single string at least...
>> Urgh. *g*
>>
>> I'm not sure if we should force every BDS to have a clearly defining
>> file name. If there are options, which completely change the
>> behavior of the block driver (I wouldn't consider lazy-refcounts one
>> of them since it doesn't change the contents of the block device),
>> I'd rather return NULL when asked for a file name. But then again,
>> maybe an ugly filename is better than none at all…
> We need filenames for backing file relationships. For example, when you
> take a live snapshot, we need to reference the old image. If you don't
> use the filename, but driver-specific options, I believe this fails
> today.
>
> You might also want to set some options for the backing file in images
> created with qemu-img.

Yes, I hoped we could use the options instead. But if it fails… Maybe 
it's worth fixing, I don't know. ;-)

> The alternative would be to extend qcow2 to have something more complex
> than a string to describe backing files. However, this would mean that
> qcow2 is the only possible format for live snapshots.

Well, the problem would arise only for backing files which can't be 
sufficiently described through a rather simple filename. If there are 
exceptions where we are indeed forced to specify some options, qemu 
would be the only program knowing how to interpret those filenames 
anyway, therefore, there is no point in trying to be compatible.

Max

>
>> In general, I'd prefer abandoning filenames* (especially protocol
>> filenames) altogether. The set of options with which to recreate the
>> same BDS is already available.
>>
>> Max
>>
>> *Of course, we need filenames for, well, opening files, but I'm
>> referring to have an explicit string "filename" in addition to the
>> option dicts (nearly) everywhere.
> Agreed. The reason why filenames are still passed separately and not
> converted to a file.filename QDict entry is the convenience magic that
> they enable (at least protocol names) and that file.filename doesn't
> have in order to have less special cases with blockdev-add.
>
> So what you'd need to do is to parse the protocol names in the top-level
> function bdrv_open() and convert them into the right QDict entries.
> Perhaps this is also a better place for the .bdrv_parse_filename()
> calls. And then you could call a new bdrv_file_open() that doesn't take
> a separate filename argument any more.
>
> Kevin

Re: [Qemu-devel] [PATCH] block: fix backing file segfault

[Kevin Wolf]

Am 10.01.2014 um 19:38 hat Max Reitz geschrieben:
> On 10.01.2014 19:26, Kevin Wolf wrote:
> >Am 10.01.2014 um 19:05 hat Max Reitz geschrieben:
> >>On 10.01.2014 18:55, Kevin Wolf wrote:
> >>>Ok, if you're happy with it, I'll apply it. Can I put your Reviewed-by
> >>>there?
> >>Yes, feel free to.
> >Thanks, applied to the block branch.
> >
> >Peter, no need for a second version of the patch then. :-)
> >
> >>>In the long run, we need to get rid of all this copying anyway. I'm
> >>>imagining a BlockDriver function that returns a file name to reproduce
> >>>the same setup, and a removal of bs->backing_file and bs->file_name.
> >>>
> >>>For some drivers, the returned filename would be a URL or some other
> >>>string that that particular driver can parse.
> >>>
> >>>While doing that, we might also consider a fake protocol that handles
> >>>filenames like 'json:{"driver":"qcow2","lazy-refcounts":"on",...}',
> >>>because for some drivers this might be the only thing that comes close
> >>>to a filename as it is a single string at least...
> >>Urgh. *g*
> >>
> >>I'm not sure if we should force every BDS to have a clearly defining
> >>file name. If there are options, which completely change the
> >>behavior of the block driver (I wouldn't consider lazy-refcounts one
> >>of them since it doesn't change the contents of the block device),
> >>I'd rather return NULL when asked for a file name. But then again,
> >>maybe an ugly filename is better than none at all…
> >We need filenames for backing file relationships. For example, when you
> >take a live snapshot, we need to reference the old image. If you don't
> >use the filename, but driver-specific options, I believe this fails
> >today.
> >
> >You might also want to set some options for the backing file in images
> >created with qemu-img.
> 
> Yes, I hoped we could use the options instead. But if it fails…
> Maybe it's worth fixing, I don't know. ;-)

You just need to make them storable in images, qcow2 at least.

The way in which I think live snapshots fail today (untested) is that
the VM continues to run happily on the new overlay image, but you can't
restart the VM because the backing file link in the image file is
missing. That's a nasty way of failure and should definitely be fixed.

> >The alternative would be to extend qcow2 to have something more complex
> >than a string to describe backing files. However, this would mean that
> >qcow2 is the only possible format for live snapshots.
> 
> Well, the problem would arise only for backing files which can't be
> sufficiently described through a rather simple filename. If there
> are exceptions where we are indeed forced to specify some options,
> qemu would be the only program knowing how to interpret those
> filenames anyway, therefore, there is no point in trying to be
> compatible.

Fair enough. Let's try to cope without a json: protocol if we can.

Kevin

Re: [Qemu-devel] [PATCH] block: fix backing file segfault

[Peter Feiner]

On Fri, Jan 10, 2014 at 1:26 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 10.01.2014 um 19:05 hat Max Reitz geschrieben:
>> On 10.01.2014 18:55, Kevin Wolf wrote:
>> >Ok, if you're happy with it, I'll apply it. Can I put your Reviewed-by
>> >there?
>>
>> Yes, feel free to.
>
> Thanks, applied to the block branch.
>
> Peter, no need for a second version of the patch then. :-)

I'll still submit v2 to add braces and incorporate the examples in
tests/qemu-iotests.

Re: [Qemu-devel] [PATCH] block: fix backing file segfault

[Kevin Wolf]

Am 10.01.2014 um 20:03 hat Peter Feiner geschrieben:
> On Fri, Jan 10, 2014 at 1:26 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> > Am 10.01.2014 um 19:05 hat Max Reitz geschrieben:
> >> On 10.01.2014 18:55, Kevin Wolf wrote:
> >> >Ok, if you're happy with it, I'll apply it. Can I put your Reviewed-by
> >> >there?
> >>
> >> Yes, feel free to.
> >
> > Thanks, applied to the block branch.
> >
> > Peter, no need for a second version of the patch then. :-)
> 
> I'll still submit v2 to add braces and incorporate the examples in
> tests/qemu-iotests.

Oh, right, that should still be done. Thanks for stopping me.

Kevin