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

[Alex Bligh]

Kevin,

--On 17 October 2012 16:45:39 +0200 Kevin Wolf <address@hidden> 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])


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).

--
Alex Bligh