Re: [Qemu-devel] [PATCHv4] qemu-img rebase: use empty string to rebase without backing file

[Kevin Wolf <kwolf@redhat.com>]

Am 18.10.2012 23:20, schrieb Alex Bligh:
> Kevin,
> 
> --On 17 October 2012 16:45:39 +0200 Kevin Wolf <kwolf@redhat.com> wrote:
> 
>> um_sectors) {
>>> @@ -1675,7 +1679,12 @@ static int img_rebase(int argc, char **argv)
>>>       * backing file are overwritten in the COW file now, so the visible content
>>>       * doesn't change when we switch the backing file.
>>>       */
>>> -    ret = bdrv_change_backing_file(bs, out_baseimg, out_basefmt);
>>> +    if (bs_new_backing) {
>>
>> I think this needs to be out_baseimg, otherwise -u is broken. I've
>> updated the patch, please check if you agree with the fix.
> 
> I'm not sure I do agree.
> 
> When -u is not specified, then unsafe=0. If the backing file is the empty
> string then bs_new_backing is 0 here, and the if condition evaluates to
> false, in the current patch.
> 
> If you make that "if (outbase_img)" then it will still evaluate to true,
> because whilst outbase_img is non-zero, outbase_img[0] is zero.
> 
> So I think you either need to do:
> 
>    if (bs_new_backing || unsafe)
> 
> which replicates the existing behaviour, or
> 
>    if (out_baseimg && out_baseimg[0])

Good point, I changed it.

> As it happens, we despite what Eric Blake said, we couldn't get an unsafe
> rebase to no backing file to work with the existing code (with our without
> our patch). The second option may fix this bug. Reading line 1497, is this
> because the semantic is not 'an empty string', but 'omit -b entirely'?
> This behaviour is undocumented in the manpage which specifies -b as a
> compulsory option. If so, that's a bit unfortunate as we now have different
> semantics with and without -u. Note if no -b parameter is supplied, there
> is also a possible null pointer exception at line 1693 (null passed to
> error_report).

Right. I think not passing -b at all or passing an empty string should
have the same meaning, namely removing the backing file reference. I
won't try to modify this patch to do this, though, we can do it on top.

Kevin