Am 18.10.2012 23:20, schrieb Alex Bligh:
> Kevin,
>
> --On 17 October 2012 16:45:39 +0200 Kevin Wolf <kw...@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
