| Message ID | 1350391578-1191-1-git-send-email-alex@alex.org.uk |
|---|---|
| State | New |
| Headers | show |
Am 16.10.2012 14:46, schrieb Alex Bligh: > This patch allows an empty filename to be passed as the new base image name > for qemu-img rebase to mean base the image on no backing file (i.e. > independent of any backing file). According to Eric Blake, qemu-img rebase > already supports this when '-u' is used; this adds support when -u is not > used. > > Signed-off-by: Alex Bligh <alex@alex.org.uk> Thanks, applied to the block branch. Kevin
Am 16.10.2012 14:46, schrieb Alex Bligh: > This patch allows an empty filename to be passed as the new base image name > for qemu-img rebase to mean base the image on no backing file (i.e. > independent of any backing file). According to Eric Blake, qemu-img rebase > already supports this when '-u' is used; this adds support when -u is not > used. > > Signed-off-by: Alex Bligh <alex@alex.org.uk> > --- > qemu-img.c | 29 +++++++++++++++++++---------- > qemu-img.texi | 4 +++- > 2 files changed, 22 insertions(+), 11 deletions(-) > > Also obtainable from: > https://github.com/abligh/qemu.git > Commit at: > https://github.com/abligh/qemu/commit/982ec105018f9901619663642de9d0a5de86799d > > diff --git a/qemu-img.c b/qemu-img.c > index f17f187..2816f37 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -1558,13 +1558,15 @@ static int img_rebase(int argc, char **argv) > error_report("Could not open old backing file '%s'", backing_name); > goto out; > } > - > - bs_new_backing = bdrv_new("new_backing"); > - ret = bdrv_open(bs_new_backing, out_baseimg, BDRV_O_FLAGS, > + if (out_baseimg[0]) { > + bs_new_backing = bdrv_new("new_backing"); > + ret = bdrv_open(bs_new_backing, out_baseimg, BDRV_O_FLAGS, > new_backing_drv); > - if (ret) { > - error_report("Could not open new backing file '%s'", out_baseimg); > - goto out; > + if (ret) { > + error_report("Could not open new backing file '%s'", > + out_baseimg); > + goto out; > + } > } > } > > @@ -1580,7 +1582,7 @@ static int img_rebase(int argc, char **argv) > if (!unsafe) { > uint64_t num_sectors; > uint64_t old_backing_num_sectors; > - uint64_t new_backing_num_sectors; > + uint64_t new_backing_num_sectors = 0; > uint64_t sector; > int n; > uint8_t * buf_old; > @@ -1592,7 +1594,9 @@ static int img_rebase(int argc, char **argv) > > bdrv_get_geometry(bs, &num_sectors); > bdrv_get_geometry(bs_old_backing, &old_backing_num_sectors); > - bdrv_get_geometry(bs_new_backing, &new_backing_num_sectors); > + if (bs_new_backing) { > + bdrv_get_geometry(bs_new_backing, &new_backing_num_sectors); > + } > > local_progress = (float)100 / > (num_sectors / MIN(num_sectors, IO_BUF_SIZE / 512)); > @@ -1629,7 +1633,7 @@ static int img_rebase(int argc, char **argv) > } > } > > - if (sector >= new_backing_num_sectors) { > + if (sector >= new_backing_num_sectors || !bs_new_backing) { > memset(buf_new, 0, n * BDRV_SECTOR_SIZE); > } else { > if (sector + n > new_backing_num_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. Kevin
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]) 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).
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
On 10/19/2012 10:46 AM, Kevin Wolf wrote: >> 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. Agreed - a future patch that makes -b optional rather than mandatory, where '-b ""' and omitting the argument have the same semantics whether or not -u is present, would be nice.
diff --git a/qemu-img.c b/qemu-img.c index f17f187..2816f37 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -1558,13 +1558,15 @@ static int img_rebase(int argc, char **argv) error_report("Could not open old backing file '%s'", backing_name); goto out; } - - bs_new_backing = bdrv_new("new_backing"); - ret = bdrv_open(bs_new_backing, out_baseimg, BDRV_O_FLAGS, + if (out_baseimg[0]) { + bs_new_backing = bdrv_new("new_backing"); + ret = bdrv_open(bs_new_backing, out_baseimg, BDRV_O_FLAGS, new_backing_drv); - if (ret) { - error_report("Could not open new backing file '%s'", out_baseimg); - goto out; + if (ret) { + error_report("Could not open new backing file '%s'", + out_baseimg); + goto out; + } } } @@ -1580,7 +1582,7 @@ static int img_rebase(int argc, char **argv) if (!unsafe) { uint64_t num_sectors; uint64_t old_backing_num_sectors; - uint64_t new_backing_num_sectors; + uint64_t new_backing_num_sectors = 0; uint64_t sector; int n; uint8_t * buf_old; @@ -1592,7 +1594,9 @@ static int img_rebase(int argc, char **argv) bdrv_get_geometry(bs, &num_sectors); bdrv_get_geometry(bs_old_backing, &old_backing_num_sectors); - bdrv_get_geometry(bs_new_backing, &new_backing_num_sectors); + if (bs_new_backing) { + bdrv_get_geometry(bs_new_backing, &new_backing_num_sectors); + } local_progress = (float)100 / (num_sectors / MIN(num_sectors, IO_BUF_SIZE / 512)); @@ -1629,7 +1633,7 @@ static int img_rebase(int argc, char **argv) } } - if (sector >= new_backing_num_sectors) { + if (sector >= new_backing_num_sectors || !bs_new_backing) { memset(buf_new, 0, n * BDRV_SECTOR_SIZE); } else { if (sector + n > new_backing_num_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) { + ret = bdrv_change_backing_file(bs, out_baseimg, out_basefmt); + } else { + ret = bdrv_change_backing_file(bs, NULL, NULL); + } + if (ret == -ENOSPC) { error_report("Could not change the backing file to '%s': No " "space left in the file header", out_baseimg); diff --git a/qemu-img.texi b/qemu-img.texi index 8b05f2c..42ec392 100644 --- a/qemu-img.texi +++ b/qemu-img.texi @@ -148,7 +148,9 @@ Changes the backing file of an image. Only the formats @code{qcow2} and The backing file is changed to @var{backing_file} and (if the image format of @var{filename} supports this) the backing file format is changed to -@var{backing_fmt}. +@var{backing_fmt}. If @var{backing_file} is specified as ``'' (the empty +string), then the image is rebased onto no backing file (i.e. it will exist +independently of any backing file). There are two different modes in which @code{rebase} can operate: @table @option
This patch allows an empty filename to be passed as the new base image name for qemu-img rebase to mean base the image on no backing file (i.e. independent of any backing file). According to Eric Blake, qemu-img rebase already supports this when '-u' is used; this adds support when -u is not used. Signed-off-by: Alex Bligh <alex@alex.org.uk> --- qemu-img.c | 29 +++++++++++++++++++---------- qemu-img.texi | 4 +++- 2 files changed, 22 insertions(+), 11 deletions(-) Also obtainable from: https://github.com/abligh/qemu.git Commit at: https://github.com/abligh/qemu/commit/982ec105018f9901619663642de9d0a5de86799d