[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] qemu-img: Check for backing image if specified
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH] qemu-img: Check for backing image if specified during create |
Date: |
Thu, 4 May 2017 14:58:58 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.0 |
On 05/04/2017 02:30 PM, John Snow wrote:
> Or, rather, force the open of a backing image if one was specified
> for creation. Using a similar -unsafe option as rebase, allow qemu-img
> to ignore the backing file validation if possible.
>
> It may not always be possible, as in the existing case of if a filesize
s/of if/when/
> for the new image was not specified.
>
> This is accomplished by shifting around the conditionals in
> bdrv_img_create, such that a backing file is always opened unless we
> provide BDRV_O_NO_BACKING. qemu-img is adjusted to pass this new flag
> when -u is provided to create.
>
> Sorry for the heinous looking diffstat, but it's mostly whitespace.
>
Indeed, reviewing under 'git diff -b' proved fruitful.
> Inspired by: https://bugzilla.redhat.com/show_bug.cgi?id=1213786
>
> Signed-off-by: John Snow <address@hidden>
> ---
> block.c | 75
> +++++++++++++++++++++++-----------------------
> qemu-img-cmds.hx | 4 +--
Conflicts with the documentation patch for [-b backing_file] (already on
PULL request:
https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg05816.html
so you'll have to rebase, but it shouldn't be too difficult.
> qemu-img.c | 15 ++++++----
> tests/qemu-iotests/082 | 4 +--
> tests/qemu-iotests/082.out | 4 +--
> 5 files changed, 54 insertions(+), 48 deletions(-)
>
> diff --git a/block.c b/block.c
> index 5db266b..da28052 100644
> --- a/block.c
> +++ b/block.c
> @@ -4280,38 +4280,38 @@ void bdrv_img_create(const char *filename, const char
> *fmt,
> // The size for the image must always be specified, with one exception:
> // If we are using a backing file, we can obtain the size from there
> size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0);
> - if (size == -1) {
Wait. How does this even work? qemu_opt_get_size() returns an unsigned
value, either what was specified by the option, or else the default
parameter (which we passed as 0), so how can size ever be -1? It might
make sense if we had called:
size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, -1);
with the intent of allowing an explicit size of 0. But I'm lost as to
how this code ever worked pre-patch...
> +
> + if (size == -1) {
...which therefore carries over to the state of things post-patch.
/me goes and reads code
What?
if (desc && desc->def_value_str) {
parse_option_size(name, desc->def_value_str, &ret, &error_abort);
return ret;
Oh - so THAT's how: qemu_opt_get_size_helper() ignores the 'defval'
input and instead returns a DIFFERENT default associated with the
QemuOpts. UGLY. But not your fault.
> +++ b/qemu-img.c
> @@ -145,9 +145,10 @@ static void QEMU_NORETURN help(void)
> " 'snapshot_id_or_name' is deprecated, use 'snapshot_param'\n"
> " instead\n"
> " '-c' indicates that target image must be compressed (qcow
> format only)\n"
> - " '-u' enables unsafe rebasing. It is assumed that old and new
> backing file\n"
> - " match exactly. The image doesn't need a working backing
> file before\n"
> - " rebasing in this case (useful for renaming the backing
> file)\n"
> + " '-u' allows unsafe backing chains. For rebasing, it is assumed
> that old and new \n"
> + " backing file match exactly. The image doesn't need a
> working backing file \n"
> + " before rebasing in this case (useful for renaming the
> backing file)\n"
> + " For image creation, allow creating without attempting to
> open the file.\n"
maybe s/to open the file/to open the backing file/
> +++ b/tests/qemu-iotests/082
> @@ -85,8 +85,8 @@ run_qemu_img create -f $IMGFMT -o cluster_size=4k -o help
> "$TEST_IMG" $size
> run_qemu_img create -f $IMGFMT -o cluster_size=4k -o \? "$TEST_IMG" $size
>
> # Looks like a help option, but is part of the backing file name
> -run_qemu_img create -f $IMGFMT -o backing_file="$TEST_IMG",,help "$TEST_IMG"
> $size
> -run_qemu_img create -f $IMGFMT -o backing_file="$TEST_IMG",,\? "$TEST_IMG"
> $size
> +run_qemu_img create -f $IMGFMT -u -o backing_file="$TEST_IMG",,help
> "$TEST_IMG" $size
> +run_qemu_img create -f $IMGFMT -u -o backing_file="$TEST_IMG",,\?
> "$TEST_IMG" $size
Nice test of the new usage.
I like where it's headed. It will need a v2 to rebase and fix the
things I pointed out, but I'm looking forward to it.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature