[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v4] qemu-img: Check for backing image if specifi
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [PATCH v4] qemu-img: Check for backing image if specified during create |
Date: |
Fri, 12 May 2017 20:07:38 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.0 |
On 2017-05-11 20:27, John Snow wrote:
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1213786
>
> 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 when a filesize
> 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.
>
> Reported-by: Yi Sun <address@hidden>
> Signed-off-by: John Snow <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>
> ---
>
> v4: Actually do the things Eric told me to.
> v3: Rebased
> v2: Rebased for 2.10
> Corrected some of my less cromulent grammar
>
>
> block.c | 73
> +++++++++++++++++++++++-----------------------
> qemu-img-cmds.hx | 4 +--
> qemu-img.c | 16 ++++++----
> tests/qemu-iotests/082 | 4 +--
> tests/qemu-iotests/082.out | 4 +--
> 5 files changed, 54 insertions(+), 47 deletions(-)
>
> diff --git a/block.c b/block.c
> index a45b9b5..3c3df54 100644
> --- a/block.c
> +++ b/block.c
> @@ -4275,37 +4275,37 @@ 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) {
"Hang on, why should this be -1 when the defval is 0? Where does the -1
come from?"
"..."
"Oh, the option exists and is set to -1? Why is that?"
"..."
"Oh, because this function always sets it itself, and because @img_size
is set to (uint64_t)-1."
First, I won't start with how signed integer overflow is
implementation-defined in C because I hope you have thrashed that out
with Eric (I hope that "to thrash out" is a good translation for
"auskaspern" (lit. "to buffoon out").).
Second, well, at least we should put -1 as the default value here, then.
Not strictly your fault or something that you need to fix, but it is
just a single line in the vicinity...
Let me know if you want to address this, for now I'll leave a
Reviewed-by: Max Reitz <address@hidden>
here if you don't want to.
Max
> - if (backing_file) {
> - BlockDriverState *bs;
> - char *full_backing = g_new0(char, PATH_MAX);
> - int64_t size;
> - int back_flags;
> - QDict *backing_options = NULL;
> -
> - bdrv_get_full_backing_filename_from_filename(filename,
> backing_file,
> - full_backing,
> PATH_MAX,
> - &local_err);
> - if (local_err) {
> - g_free(full_backing);
> - goto out;
> - }
> -
> - /* backing files always opened read-only */
> - back_flags = flags;
> - back_flags &= ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT |
> BDRV_O_NO_BACKING);
> -
> - if (backing_fmt) {
> - backing_options = qdict_new();
> - qdict_put_str(backing_options, "driver", backing_fmt);
> - }
> -
> - bs = bdrv_open(full_backing, NULL, backing_options, back_flags,
> - &local_err);
> + if (backing_file && !(flags & BDRV_O_NO_BACKING)) {
> + BlockDriverState *bs;
> + char *full_backing = g_new0(char, PATH_MAX);
> + int back_flags;
> + QDict *backing_options = NULL;
> +
> + bdrv_get_full_backing_filename_from_filename(filename, backing_file,
> + full_backing, PATH_MAX,
> + &local_err);
> + if (local_err) {
> g_free(full_backing);
> - if (!bs) {
> - goto out;
> - }
> + goto out;
> + }
> +
> + /* backing files always opened read-only */
> + back_flags = flags;
> + back_flags &= ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
> +
> + if (backing_fmt) {
> + backing_options = qdict_new();
> + qdict_put_str(backing_options, "driver", backing_fmt);
> + }
> +
> + bs = bdrv_open(full_backing, NULL, backing_options, back_flags,
> + &local_err);
> + g_free(full_backing);
> + if (!bs) {
> + goto out;
> + }
> +
> + if (size == -1) {
> size = bdrv_getlength(bs);
> if (size < 0) {
> error_setg_errno(errp, -size, "Could not get size of '%s'",
> @@ -4313,14 +4313,15 @@ void bdrv_img_create(const char *filename, const char
> *fmt,
> bdrv_unref(bs);
> goto out;
> }
> -
> qemu_opt_set_number(opts, BLOCK_OPT_SIZE, size, &error_abort);
> -
> - bdrv_unref(bs);
> - } else {
> - error_setg(errp, "Image creation needs a size parameter");
> - goto out;
> }
> +
> + bdrv_unref(bs);
> + }
> +
> + if (size == -1) {
> + error_setg(errp, "Image creation needs a size parameter");
> + goto out;
> }
>
> if (!quiet) {
> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
> index bf4ce59..1d230c6 100644
> --- a/qemu-img-cmds.hx
> +++ b/qemu-img-cmds.hx
> @@ -22,9 +22,9 @@ STEXI
> ETEXI
>
> DEF("create", img_create,
> - "create [-q] [--object objectdef] [-f fmt] [-b backing_file] [-F
> backing_fmt] [-o options] filename [size]")
> + "create [-q] [--object objectdef] [-f fmt] [-b backing_file] [-F
> backing_fmt] [-u] [-o options] filename [size]")
> STEXI
> address@hidden create [--object @var{objectdef}] [-q] [-f @var{fmt}] [-b
> @var{backing_file}] [-F @var{backing_fmt}] [-o @var{options}] @var{filename}
> address@hidden
> address@hidden create [--object @var{objectdef}] [-q] [-f @var{fmt}] [-b
> @var{backing_file}] [-F @var{backing_fmt}] [-u] [-o @var{options}]
> @var{filename} address@hidden
> ETEXI
>
> DEF("commit", img_commit,
> diff --git a/qemu-img.c b/qemu-img.c
> index f3b0ab4..dd977a8 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -145,9 +145,11 @@ 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\n"
> + " new backing file match exactly. The image doesn't need a
> working\n"
> + " backing file before rebasing in this case (useful for
> renaming the\n"
> + " backing file). For image creation, allow creating without
> attempting\n"
> + " to open the backing file.\n"
> " '-h' with or without a command shows this help and lists the
> supported formats\n"
> " '-p' show progress of command (only certain commands)\n"
> " '-q' use Quiet mode - do not print any output (except
> errors)\n"
> @@ -409,6 +411,7 @@ static int img_create(int argc, char **argv)
> char *options = NULL;
> Error *local_err = NULL;
> bool quiet = false;
> + int flags = 0;
>
> for(;;) {
> static const struct option long_options[] = {
> @@ -416,7 +419,7 @@ static int img_create(int argc, char **argv)
> {"object", required_argument, 0, OPTION_OBJECT},
> {0, 0, 0, 0}
> };
> - c = getopt_long(argc, argv, ":F:b:f:he6o:q",
> + c = getopt_long(argc, argv, ":F:b:f:he6o:qu",
> long_options, NULL);
> if (c == -1) {
> break;
> @@ -464,6 +467,9 @@ static int img_create(int argc, char **argv)
> case 'q':
> quiet = true;
> break;
> + case 'u':
> + flags |= BDRV_O_NO_BACKING;
> + break;
> case OPTION_OBJECT: {
> QemuOpts *opts;
> opts = qemu_opts_parse_noisily(&qemu_object_opts,
> @@ -516,7 +522,7 @@ static int img_create(int argc, char **argv)
> }
>
> bdrv_img_create(filename, fmt, base_filename, base_fmt,
> - options, img_size, 0, quiet, &local_err);
> + options, img_size, flags, quiet, &local_err);
> if (local_err) {
> error_reportf_err(local_err, "%s: ", filename);
> goto fail;
> diff --git a/tests/qemu-iotests/082 b/tests/qemu-iotests/082
> index ad1d9fa..d5c83d4 100755
> --- a/tests/qemu-iotests/082
> +++ 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
>
> # Try to trick qemu-img into creating escaped commas
> run_qemu_img create -f $IMGFMT -o backing_file="$TEST_IMG", -o help
> "$TEST_IMG" $size
> diff --git a/tests/qemu-iotests/082.out b/tests/qemu-iotests/082.out
> index a952330..f2732e4 100644
> --- a/tests/qemu-iotests/082.out
> +++ b/tests/qemu-iotests/082.out
> @@ -146,10 +146,10 @@ lazy_refcounts Postpone refcount updates
> refcount_bits Width of a reference count entry in bits
> nocow Turn off copy-on-write (valid only on btrfs)
>
> -Testing: create -f qcow2 -o backing_file=TEST_DIR/t.qcow2,,help
> TEST_DIR/t.qcow2 128M
> +Testing: create -f qcow2 -u -o backing_file=TEST_DIR/t.qcow2,,help
> TEST_DIR/t.qcow2 128M
> Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=134217728
> backing_file=TEST_DIR/t.qcow2,,help encryption=off cluster_size=65536
> lazy_refcounts=off refcount_bits=16
>
> -Testing: create -f qcow2 -o backing_file=TEST_DIR/t.qcow2,,?
> TEST_DIR/t.qcow2 128M
> +Testing: create -f qcow2 -u -o backing_file=TEST_DIR/t.qcow2,,?
> TEST_DIR/t.qcow2 128M
> Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=134217728
> backing_file=TEST_DIR/t.qcow2,,? encryption=off cluster_size=65536
> lazy_refcounts=off refcount_bits=16
>
> Testing: create -f qcow2 -o backing_file=TEST_DIR/t.qcow2, -o help
> TEST_DIR/t.qcow2 128M
>
signature.asc
Description: OpenPGP digital signature
- [Qemu-block] [PATCH v4] qemu-img: Check for backing image if specified during create, John Snow, 2017/05/11
- Re: [Qemu-block] [PATCH v4] qemu-img: Check for backing image if specified during create,
Max Reitz <=
- Re: [Qemu-block] [PATCH v4] qemu-img: Check for backing image if specified during create, Eric Blake, 2017/05/12
- Re: [Qemu-block] [PATCH v4] qemu-img: Check for backing image if specified during create, John Snow, 2017/05/12
- Re: [Qemu-block] [PATCH v4] qemu-img: Check for backing image if specified during create, Max Reitz, 2017/05/15
- Re: [Qemu-block] [PATCH v4] qemu-img: Check for backing image if specified during create, Max Reitz, 2017/05/15
- Re: [Qemu-block] [PATCH v4] qemu-img: Check for backing image if specified during create, Kevin Wolf, 2017/05/16
- Re: [Qemu-block] [PATCH v4] qemu-img: Check for backing image if specified during create, Max Reitz, 2017/05/17