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(-)
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 <yisun@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
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) {
- 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
-@item create [--object @var{objectdef}] [-q] [-f @var{fmt}] [-b @var{backing_file}] [-F @var{backing_fmt}] [-o @var{options}] @var{filename} [@var{size}]
+@item create [--object @var{objectdef}] [-q] [-f @var{fmt}] [-b @var{backing_file}] [-F @var{backing_fmt}] [-u] [-o @var{options}] @var{filename} [@var{size}]
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
--
2.9.3
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 <yisun@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>
> 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 <mreitz@redhat.com>
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
> -@item create [--object @var{objectdef}] [-q] [-f @var{fmt}] [-b @var{backing_file}] [-F @var{backing_fmt}] [-o @var{options}] @var{filename} [@var{size}]
> +@item create [--object @var{objectdef}] [-q] [-f @var{fmt}] [-b @var{backing_file}] [-F @var{backing_fmt}] [-u] [-o @var{options}] @var{filename} [@var{size}]
> 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
>
On 05/12/2017 01:07 PM, Max Reitz wrote:
> 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.
>>
>> +++ 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."
I had pretty much the same conversation on my v1 review.
https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg01097.html
>
> 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").).
Sounds like a reasonable choice of words, even if I don't speak the
counterpart language to validate your translation.
(uint64_t)-1 is well-defined in C (so I think we're just fine here). But
(int64_t)UINT64_MAX is where signed integer overflow does indeed throw
wrinkles at you.
I seem to recall that qemu has chosen to use compiler flags and/or
assumptions that we are using 2s-complement arithmetic with sane
behavior (that is, tighter behavior than the bare minimum that C
requires), because it was easier than auditing our code for strict C
compliance on border cases of conversions from unsigned to signed that
trigger undefined behavior. But again, I don't think it affects this
patch (where our conversion is only from signed to unsigned, and that is
well-defined behavior).
>
> Second, well, at least we should put -1 as the default value here, then.
Indeed, now that two reviewers have tripped on it,
qemu_opt_get_size(,,-1) would be nicer.
>
> 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 <mreitz@redhat.com>
>
> here if you don't want to.
I'm okay whether you want to squash that fix into this patch, or whether
you do it as a separate followup patch.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
On 05/12/2017 03:46 PM, Eric Blake wrote:
> On 05/12/2017 01:07 PM, Max Reitz wrote:
>> 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.
>>>
>
>>> +++ 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."
>
> I had pretty much the same conversation on my v1 review.
> https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg01097.html
>
>>
>> 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").).
>
> Sounds like a reasonable choice of words, even if I don't speak the
> counterpart language to validate your translation.
>
> (uint64_t)-1 is well-defined in C (so I think we're just fine here). But
> (int64_t)UINT64_MAX is where signed integer overflow does indeed throw
> wrinkles at you.
>
> I seem to recall that qemu has chosen to use compiler flags and/or
> assumptions that we are using 2s-complement arithmetic with sane
> behavior (that is, tighter behavior than the bare minimum that C
> requires), because it was easier than auditing our code for strict C
> compliance on border cases of conversions from unsigned to signed that
> trigger undefined behavior. But again, I don't think it affects this
> patch (where our conversion is only from signed to unsigned, and that is
> well-defined behavior).
>
>
>>
>> Second, well, at least we should put -1 as the default value here, then.
>
> Indeed, now that two reviewers have tripped on it,
> qemu_opt_get_size(,,-1) would be nicer.
>
>>
>> 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 <mreitz@redhat.com>
>>
>> here if you don't want to.
>
> I'm okay whether you want to squash that fix into this patch, or whether
> you do it as a separate followup patch.
>
I had considered the issue separate; but you're welcome to either write
a patch or squish it into this one, I'm not going to be picky.
--js
On 2017-05-12 21:47, John Snow wrote:
>
>
> On 05/12/2017 03:46 PM, Eric Blake wrote:
>> On 05/12/2017 01:07 PM, Max Reitz wrote:
>>> 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.
>>>>
>>
>>>> +++ 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."
>>
>> I had pretty much the same conversation on my v1 review.
>> https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg01097.html
>>
>>>
>>> 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").).
>>
>> Sounds like a reasonable choice of words, even if I don't speak the
>> counterpart language to validate your translation.
>>
>> (uint64_t)-1 is well-defined in C (so I think we're just fine here). But
>> (int64_t)UINT64_MAX is where signed integer overflow does indeed throw
>> wrinkles at you.
We're not really fine because that conversion happens when the result of
qemu_opt_get_size() (a uint64_t) is stored in size (an int64_t).
>> I seem to recall that qemu has chosen to use compiler flags and/or
>> assumptions that we are using 2s-complement arithmetic with sane
>> behavior (that is, tighter behavior than the bare minimum that C
>> requires), because it was easier than auditing our code for strict C
>> compliance on border cases of conversions from unsigned to signed that
>> trigger undefined behavior. But again, I don't think it affects this
>> patch (where our conversion is only from signed to unsigned, and that is
>> well-defined behavior).
Right. Which is why I said I won't even start on it, but of course I
did. O:-)
>>> Second, well, at least we should put -1 as the default value here, then.
>>
>> Indeed, now that two reviewers have tripped on it,
>> qemu_opt_get_size(,,-1) would be nicer.
>>
>>>
>>> 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 <mreitz@redhat.com>
>>>
>>> here if you don't want to.
>>
>> I'm okay whether you want to squash that fix into this patch, or whether
>> you do it as a separate followup patch.
>>
>
> I had considered the issue separate; but you're welcome to either write
> a patch or squish it into this one, I'm not going to be picky.
Yep, it is a separate issue, just related. :-)
But since you and Eric agree, I've squashed it in and thus I'm more than
glad to thank you very much and announce this patch as applied to my
block branch:
https://github.com/XanClic/qemu/commits/block
Max
On 2017-05-15 20:41, Max Reitz wrote:
> On 2017-05-12 21:47, John Snow wrote:
>>
>>
>> On 05/12/2017 03:46 PM, Eric Blake wrote:
>>> On 05/12/2017 01:07 PM, Max Reitz wrote:
>>>> 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.
>>>>>
>>>
>>>>> +++ 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."
>>>
>>> I had pretty much the same conversation on my v1 review.
>>> https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg01097.html
>>>
>>>>
>>>> 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").).
>>>
>>> Sounds like a reasonable choice of words, even if I don't speak the
>>> counterpart language to validate your translation.
>>>
>>> (uint64_t)-1 is well-defined in C (so I think we're just fine here). But
>>> (int64_t)UINT64_MAX is where signed integer overflow does indeed throw
>>> wrinkles at you.
>
> We're not really fine because that conversion happens when the result of
> qemu_opt_get_size() (a uint64_t) is stored in size (an int64_t).
>
>>> I seem to recall that qemu has chosen to use compiler flags and/or
>>> assumptions that we are using 2s-complement arithmetic with sane
>>> behavior (that is, tighter behavior than the bare minimum that C
>>> requires), because it was easier than auditing our code for strict C
>>> compliance on border cases of conversions from unsigned to signed that
>>> trigger undefined behavior. But again, I don't think it affects this
>>> patch (where our conversion is only from signed to unsigned, and that is
>>> well-defined behavior).
>
> Right. Which is why I said I won't even start on it, but of course I
> did. O:-)
>
>>>> Second, well, at least we should put -1 as the default value here, then.
>>>
>>> Indeed, now that two reviewers have tripped on it,
>>> qemu_opt_get_size(,,-1) would be nicer.
>>>
>>>>
>>>> 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 <mreitz@redhat.com>
>>>>
>>>> here if you don't want to.
>>>
>>> I'm okay whether you want to squash that fix into this patch, or whether
>>> you do it as a separate followup patch.
>>>
>>
>> I had considered the issue separate; but you're welcome to either write
>> a patch or squish it into this one, I'm not going to be picky.
>
> Yep, it is a separate issue, just related. :-)
>
> But since you and Eric agree, I've squashed it in and thus I'm more than
> glad to thank you very much and announce this patch as applied to my
> block branch:
>
> https://github.com/XanClic/qemu/commits/block
...well, so much for that. I'll have to unstage it again because it
breaks a bunch of iotests (41 85 96 118 139 141 144 155 156) due to
failing to acquire image locks. :-/
I suspect this is because the backing file is opened somewhere and
trying to open it breaks now with the locking series applied.
Max
Am 15.05.2017 um 21:17 hat Max Reitz geschrieben:
> On 2017-05-15 20:41, Max Reitz wrote:
> > On 2017-05-12 21:47, John Snow wrote:
> >>
> >>
> >> On 05/12/2017 03:46 PM, Eric Blake wrote:
> >>> On 05/12/2017 01:07 PM, Max Reitz wrote:
> >>>> 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.
> >>>>>
> >>>
> >>>>> +++ 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."
> >>>
> >>> I had pretty much the same conversation on my v1 review.
> >>> https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg01097.html
> >>>
> >>>>
> >>>> 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").).
> >>>
> >>> Sounds like a reasonable choice of words, even if I don't speak the
> >>> counterpart language to validate your translation.
> >>>
> >>> (uint64_t)-1 is well-defined in C (so I think we're just fine here). But
> >>> (int64_t)UINT64_MAX is where signed integer overflow does indeed throw
> >>> wrinkles at you.
> >
> > We're not really fine because that conversion happens when the result of
> > qemu_opt_get_size() (a uint64_t) is stored in size (an int64_t).
> >
> >>> I seem to recall that qemu has chosen to use compiler flags and/or
> >>> assumptions that we are using 2s-complement arithmetic with sane
> >>> behavior (that is, tighter behavior than the bare minimum that C
> >>> requires), because it was easier than auditing our code for strict C
> >>> compliance on border cases of conversions from unsigned to signed that
> >>> trigger undefined behavior. But again, I don't think it affects this
> >>> patch (where our conversion is only from signed to unsigned, and that is
> >>> well-defined behavior).
> >
> > Right. Which is why I said I won't even start on it, but of course I
> > did. O:-)
> >
> >>>> Second, well, at least we should put -1 as the default value here, then.
> >>>
> >>> Indeed, now that two reviewers have tripped on it,
> >>> qemu_opt_get_size(,,-1) would be nicer.
> >>>
> >>>>
> >>>> 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 <mreitz@redhat.com>
> >>>>
> >>>> here if you don't want to.
> >>>
> >>> I'm okay whether you want to squash that fix into this patch, or whether
> >>> you do it as a separate followup patch.
> >>>
> >>
> >> I had considered the issue separate; but you're welcome to either write
> >> a patch or squish it into this one, I'm not going to be picky.
> >
> > Yep, it is a separate issue, just related. :-)
> >
> > But since you and Eric agree, I've squashed it in and thus I'm more than
> > glad to thank you very much and announce this patch as applied to my
> > block branch:
> >
> > https://github.com/XanClic/qemu/commits/block
>
> ...well, so much for that. I'll have to unstage it again because it
> breaks a bunch of iotests (41 85 96 118 139 141 144 155 156) due to
> failing to acquire image locks. :-/
>
> I suspect this is because the backing file is opened somewhere and
> trying to open it breaks now with the locking series applied.
I think we can legitimately set force-shared=on for opening the backing
file when testing whether the file exists.
Kevin
On 2017-05-16 10:17, Kevin Wolf wrote:
> Am 15.05.2017 um 21:17 hat Max Reitz geschrieben:
>> On 2017-05-15 20:41, Max Reitz wrote:
>>> On 2017-05-12 21:47, John Snow wrote:
>>>>
>>>>
>>>> On 05/12/2017 03:46 PM, Eric Blake wrote:
>>>>> On 05/12/2017 01:07 PM, Max Reitz wrote:
>>>>>> 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.
>>>>>>>
>>>>>
>>>>>>> +++ 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."
>>>>>
>>>>> I had pretty much the same conversation on my v1 review.
>>>>> https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg01097.html
>>>>>
>>>>>>
>>>>>> 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").).
>>>>>
>>>>> Sounds like a reasonable choice of words, even if I don't speak the
>>>>> counterpart language to validate your translation.
>>>>>
>>>>> (uint64_t)-1 is well-defined in C (so I think we're just fine here). But
>>>>> (int64_t)UINT64_MAX is where signed integer overflow does indeed throw
>>>>> wrinkles at you.
>>>
>>> We're not really fine because that conversion happens when the result of
>>> qemu_opt_get_size() (a uint64_t) is stored in size (an int64_t).
>>>
>>>>> I seem to recall that qemu has chosen to use compiler flags and/or
>>>>> assumptions that we are using 2s-complement arithmetic with sane
>>>>> behavior (that is, tighter behavior than the bare minimum that C
>>>>> requires), because it was easier than auditing our code for strict C
>>>>> compliance on border cases of conversions from unsigned to signed that
>>>>> trigger undefined behavior. But again, I don't think it affects this
>>>>> patch (where our conversion is only from signed to unsigned, and that is
>>>>> well-defined behavior).
>>>
>>> Right. Which is why I said I won't even start on it, but of course I
>>> did. O:-)
>>>
>>>>>> Second, well, at least we should put -1 as the default value here, then.
>>>>>
>>>>> Indeed, now that two reviewers have tripped on it,
>>>>> qemu_opt_get_size(,,-1) would be nicer.
>>>>>
>>>>>>
>>>>>> 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 <mreitz@redhat.com>
>>>>>>
>>>>>> here if you don't want to.
>>>>>
>>>>> I'm okay whether you want to squash that fix into this patch, or whether
>>>>> you do it as a separate followup patch.
>>>>>
>>>>
>>>> I had considered the issue separate; but you're welcome to either write
>>>> a patch or squish it into this one, I'm not going to be picky.
>>>
>>> Yep, it is a separate issue, just related. :-)
>>>
>>> But since you and Eric agree, I've squashed it in and thus I'm more than
>>> glad to thank you very much and announce this patch as applied to my
>>> block branch:
>>>
>>> https://github.com/XanClic/qemu/commits/block
>>
>> ...well, so much for that. I'll have to unstage it again because it
>> breaks a bunch of iotests (41 85 96 118 139 141 144 155 156) due to
>> failing to acquire image locks. :-/
>>
>> I suspect this is because the backing file is opened somewhere and
>> trying to open it breaks now with the locking series applied.
>
> I think we can legitimately set force-shared=on for opening the backing
> file when testing whether the file exists.
For testing whether the file exists, probably. I wouldn't call it
legitimately because I don't like making that the default at all, but it
should work.
But then we have to differentiate between testing whether the file
exists and opening it to read its size; I'm even more wary regarding the
latter.
All in all, I've stated my opinion on this matter on IRC: I don't know
how much we need this patch. It's nice to have, it's convenient to know
you have mistyped the backing filename before you try to use the image
in qemu, but it's not critical. I don't consider the current behavior a
bug per-se, it's just counterintuitive behavior (that will not cut your
head off if you don't know it, though!).
(Also, we have a lot of counterintuitive behavior. See this:
$ qemu-img create -f qcow2 sub/backing.qcow2 64M
Formatting 'sub/backing.qcow2', fmt=qcow2 size=67108864 encryption=off
cluster_size=65536 lazy_refcounts=off refcount_bits=16
$ qemu-img create -f qcow2 -b sub/backing.qcow2 sub/top.qcow2
qemu-img: sub/top.qcow2: Could not open 'sub/sub/backing.qcow2': No such
file or directory
Yeah, sure, you'll know after you've done the mistake once. But the same
applies here, too, doesn't it...?)
So for me, it's a question of convenience: How much does the current
behavior annoy users? How much annoyance will changing the behavior
bring? I'm not (or rather, no longer) convinced the former outweighs the
latter, especially since it means having to change management tools
(which create external snapshots with qemu-img create while the backing
image is in use by qemu).
If you think otherwise, well, you're the main maintainer. :-)
Max
PS: Can there be a middle ground? Like just printing a warning if the
backing file cannot be opened and we don't absolutely have to?
On 05/17/2017 07:38 AM, Max Reitz 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. >>>>>>>> >>>>>> >>> I suspect this is because the backing file is opened somewhere and >>> trying to open it breaks now with the locking series applied. >> >> I think we can legitimately set force-shared=on for opening the backing >> file when testing whether the file exists. > > For testing whether the file exists, probably. I wouldn't call it > legitimately because I don't like making that the default at all, but it > should work. > > But then we have to differentiate between testing whether the file > exists and opening it to read its size; I'm even more wary regarding the > latter. > > All in all, I've stated my opinion on this matter on IRC: I don't know > how much we need this patch. It's nice to have, it's convenient to know > you have mistyped the backing filename before you try to use the image > in qemu, but it's not critical. I don't consider the current behavior a > bug per-se, it's just counterintuitive behavior (that will not cut your > head off if you don't know it, though!). > The fact that this topic came up on the mailing list again means we should probably consider something along these lines for 2.10. > (Also, we have a lot of counterintuitive behavior. See this: > > $ qemu-img create -f qcow2 sub/backing.qcow2 64M > Formatting 'sub/backing.qcow2', fmt=qcow2 size=67108864 encryption=off > cluster_size=65536 lazy_refcounts=off refcount_bits=16 > $ qemu-img create -f qcow2 -b sub/backing.qcow2 sub/top.qcow2 > qemu-img: sub/top.qcow2: Could not open 'sub/sub/backing.qcow2': No such > file or directory > > Yeah, sure, you'll know after you've done the mistake once. But the same > applies here, too, doesn't it...?) > > So for me, it's a question of convenience: How much does the current > behavior annoy users? How much annoyance will changing the behavior > bring? I'm not (or rather, no longer) convinced the former outweighs the > latter, especially since it means having to change management tools > (which create external snapshots with qemu-img create while the backing > image is in use by qemu). > > If you think otherwise, well, you're the main maintainer. :-) > > Max > > > PS: Can there be a middle ground? Like just printing a warning if the > backing file cannot be opened and we don't absolutely have to? Middle ground may be harder, but may be nicer (especially since we are talking about tightening behavior, making things that used to work now fail is not nice if there is not a transition period where it first just warns). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Am 12.07.2017 um 16:42 hat Eric Blake geschrieben: > On 05/17/2017 07:38 AM, Max Reitz 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. > >>>>>>>> > >>>>>> > > >>> I suspect this is because the backing file is opened somewhere and > >>> trying to open it breaks now with the locking series applied. > >> > >> I think we can legitimately set force-shared=on for opening the backing > >> file when testing whether the file exists. > > > > For testing whether the file exists, probably. I wouldn't call it > > legitimately because I don't like making that the default at all, but it > > should work. > > > > But then we have to differentiate between testing whether the file > > exists and opening it to read its size; I'm even more wary regarding the > > latter. > > > > All in all, I've stated my opinion on this matter on IRC: I don't know > > how much we need this patch. It's nice to have, it's convenient to know > > you have mistyped the backing filename before you try to use the image > > in qemu, but it's not critical. I don't consider the current behavior a > > bug per-se, it's just counterintuitive behavior (that will not cut your > > head off if you don't know it, though!). > > > > The fact that this topic came up on the mailing list again means we > should probably consider something along these lines for 2.10. > > > (Also, we have a lot of counterintuitive behavior. See this: > > > > $ qemu-img create -f qcow2 sub/backing.qcow2 64M > > Formatting 'sub/backing.qcow2', fmt=qcow2 size=67108864 encryption=off > > cluster_size=65536 lazy_refcounts=off refcount_bits=16 > > $ qemu-img create -f qcow2 -b sub/backing.qcow2 sub/top.qcow2 > > qemu-img: sub/top.qcow2: Could not open 'sub/sub/backing.qcow2': No such > > file or directory > > > > Yeah, sure, you'll know after you've done the mistake once. But the same > > applies here, too, doesn't it...?) > > > > So for me, it's a question of convenience: How much does the current > > behavior annoy users? How much annoyance will changing the behavior > > bring? I'm not (or rather, no longer) convinced the former outweighs the > > latter, especially since it means having to change management tools > > (which create external snapshots with qemu-img create while the backing > > image is in use by qemu). > > > > If you think otherwise, well, you're the main maintainer. :-) > > > > Max > > > > > > PS: Can there be a middle ground? Like just printing a warning if the > > backing file cannot be opened and we don't absolutely have to? > > Middle ground may be harder, but may be nicer (especially since we are > talking about tightening behavior, making things that used to work now > fail is not nice if there is not a transition period where it first just > warns). We can do the nowadays usual thing: Add a -u option, print a deprecation warning if we don't have -u and can't open the backing file, and put it into the list of things to remove in 3.0. Kevin
On 2017-07-12 16:56, Kevin Wolf wrote: > Am 12.07.2017 um 16:42 hat Eric Blake geschrieben: >> On 05/17/2017 07:38 AM, Max Reitz 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. >>>>>>>>>> >>>>>>>> >> >>>>> I suspect this is because the backing file is opened somewhere and >>>>> trying to open it breaks now with the locking series applied. >>>> >>>> I think we can legitimately set force-shared=on for opening the backing >>>> file when testing whether the file exists. >>> >>> For testing whether the file exists, probably. I wouldn't call it >>> legitimately because I don't like making that the default at all, but it >>> should work. >>> >>> But then we have to differentiate between testing whether the file >>> exists and opening it to read its size; I'm even more wary regarding the >>> latter. >>> >>> All in all, I've stated my opinion on this matter on IRC: I don't know >>> how much we need this patch. It's nice to have, it's convenient to know >>> you have mistyped the backing filename before you try to use the image >>> in qemu, but it's not critical. I don't consider the current behavior a >>> bug per-se, it's just counterintuitive behavior (that will not cut your >>> head off if you don't know it, though!). >>> >> >> The fact that this topic came up on the mailing list again means we >> should probably consider something along these lines for 2.10. >> >>> (Also, we have a lot of counterintuitive behavior. See this: >>> >>> $ qemu-img create -f qcow2 sub/backing.qcow2 64M >>> Formatting 'sub/backing.qcow2', fmt=qcow2 size=67108864 encryption=off >>> cluster_size=65536 lazy_refcounts=off refcount_bits=16 >>> $ qemu-img create -f qcow2 -b sub/backing.qcow2 sub/top.qcow2 >>> qemu-img: sub/top.qcow2: Could not open 'sub/sub/backing.qcow2': No such >>> file or directory >>> >>> Yeah, sure, you'll know after you've done the mistake once. But the same >>> applies here, too, doesn't it...?) >>> >>> So for me, it's a question of convenience: How much does the current >>> behavior annoy users? How much annoyance will changing the behavior >>> bring? I'm not (or rather, no longer) convinced the former outweighs the >>> latter, especially since it means having to change management tools >>> (which create external snapshots with qemu-img create while the backing >>> image is in use by qemu). >>> >>> If you think otherwise, well, you're the main maintainer. :-) >>> >>> Max >>> >>> >>> PS: Can there be a middle ground? Like just printing a warning if the >>> backing file cannot be opened and we don't absolutely have to? >> >> Middle ground may be harder, but may be nicer (especially since we are >> talking about tightening behavior, making things that used to work now >> fail is not nice if there is not a transition period where it first just >> warns). > > We can do the nowadays usual thing: Add a -u option, print a deprecation > warning if we don't have -u and can't open the backing file, and put > it into the list of things to remove in 3.0. Works for me. Max
On Wed, Jul 12, 2017 at 8:00 PM Max Reitz <mreitz@redhat.com> wrote:
> On 2017-07-12 16:56, Kevin Wolf wrote:
> > Am 12.07.2017 um 16:42 hat Eric Blake geschrieben:
> >> On 05/17/2017 07:38 AM, Max Reitz 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.
> >>>>>>>>>>
> >>>>>>>>
> >>
> >>>>> I suspect this is because the backing file is opened somewhere and
> >>>>> trying to open it breaks now with the locking series applied.
> >>>>
> >>>> I think we can legitimately set force-shared=on for opening the
> backing
> >>>> file when testing whether the file exists.
> >>>
> >>> For testing whether the file exists, probably. I wouldn't call it
> >>> legitimately because I don't like making that the default at all, but
> it
> >>> should work.
> >>>
> >>> But then we have to differentiate between testing whether the file
> >>> exists and opening it to read its size; I'm even more wary regarding
> the
> >>> latter.
> >>>
> >>> All in all, I've stated my opinion on this matter on IRC: I don't know
> >>> how much we need this patch. It's nice to have, it's convenient to know
> >>> you have mistyped the backing filename before you try to use the image
> >>> in qemu, but it's not critical. I don't consider the current behavior a
> >>> bug per-se, it's just counterintuitive behavior (that will not cut your
> >>> head off if you don't know it, though!).
> >>>
> >>
> >> The fact that this topic came up on the mailing list again means we
> >> should probably consider something along these lines for 2.10.
> >>
> >>> (Also, we have a lot of counterintuitive behavior. See this:
> >>>
> >>> $ qemu-img create -f qcow2 sub/backing.qcow2 64M
> >>> Formatting 'sub/backing.qcow2', fmt=qcow2 size=67108864 encryption=off
> >>> cluster_size=65536 lazy_refcounts=off refcount_bits=16
> >>> $ qemu-img create -f qcow2 -b sub/backing.qcow2 sub/top.qcow2
> >>> qemu-img: sub/top.qcow2: Could not open 'sub/sub/backing.qcow2': No
> such
> >>> file or directory
> >>>
> >>> Yeah, sure, you'll know after you've done the mistake once. But the
> same
> >>> applies here, too, doesn't it...?)
> >>>
> >>> So for me, it's a question of convenience: How much does the current
> >>> behavior annoy users? How much annoyance will changing the behavior
> >>> bring? I'm not (or rather, no longer) convinced the former outweighs
> the
> >>> latter, especially since it means having to change management tools
> >>> (which create external snapshots with qemu-img create while the backing
> >>> image is in use by qemu).
> >>>
> >>> If you think otherwise, well, you're the main maintainer. :-)
> >>>
> >>> Max
> >>>
> >>>
> >>> PS: Can there be a middle ground? Like just printing a warning if the
> >>> backing file cannot be opened and we don't absolutely have to?
> >>
> >> Middle ground may be harder, but may be nicer (especially since we are
> >> talking about tightening behavior, making things that used to work now
> >> fail is not nice if there is not a transition period where it first just
> >> warns).
> >
> > We can do the nowadays usual thing: Add a -u option, print a deprecation
> > warning if we don't have -u and can't open the backing file, and put
> > it into the list of things to remove in 3.0.
>
> Works for me.
>
Should work for oVirt.
When we consume the -u/--unsafe option, should we use qemu-img version,
or maybe we need an official api for detecting capabilities?
qemu-img caps --format json
{
"unsafe-create": True,
...
Nir
On 07/12/2017 01:00 PM, Max Reitz wrote: > On 2017-07-12 16:56, Kevin Wolf wrote: >> Am 12.07.2017 um 16:42 hat Eric Blake geschrieben: >>> On 05/17/2017 07:38 AM, Max Reitz 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. >>>>>>>>>>> >>>>>>>>> >>> >>>>>> I suspect this is because the backing file is opened somewhere and >>>>>> trying to open it breaks now with the locking series applied. >>>>> >>>>> I think we can legitimately set force-shared=on for opening the backing >>>>> file when testing whether the file exists. >>>> >>>> For testing whether the file exists, probably. I wouldn't call it >>>> legitimately because I don't like making that the default at all, but it >>>> should work. >>>> >>>> But then we have to differentiate between testing whether the file >>>> exists and opening it to read its size; I'm even more wary regarding the >>>> latter. >>>> >>>> All in all, I've stated my opinion on this matter on IRC: I don't know >>>> how much we need this patch. It's nice to have, it's convenient to know >>>> you have mistyped the backing filename before you try to use the image >>>> in qemu, but it's not critical. I don't consider the current behavior a >>>> bug per-se, it's just counterintuitive behavior (that will not cut your >>>> head off if you don't know it, though!). >>>> >>> >>> The fact that this topic came up on the mailing list again means we >>> should probably consider something along these lines for 2.10. >>> >>>> (Also, we have a lot of counterintuitive behavior. See this: >>>> >>>> $ qemu-img create -f qcow2 sub/backing.qcow2 64M >>>> Formatting 'sub/backing.qcow2', fmt=qcow2 size=67108864 encryption=off >>>> cluster_size=65536 lazy_refcounts=off refcount_bits=16 >>>> $ qemu-img create -f qcow2 -b sub/backing.qcow2 sub/top.qcow2 >>>> qemu-img: sub/top.qcow2: Could not open 'sub/sub/backing.qcow2': No such >>>> file or directory >>>> >>>> Yeah, sure, you'll know after you've done the mistake once. But the same >>>> applies here, too, doesn't it...?) >>>> >>>> So for me, it's a question of convenience: How much does the current >>>> behavior annoy users? How much annoyance will changing the behavior >>>> bring? I'm not (or rather, no longer) convinced the former outweighs the >>>> latter, especially since it means having to change management tools >>>> (which create external snapshots with qemu-img create while the backing >>>> image is in use by qemu). >>>> >>>> If you think otherwise, well, you're the main maintainer. :-) >>>> >>>> Max >>>> >>>> >>>> PS: Can there be a middle ground? Like just printing a warning if the >>>> backing file cannot be opened and we don't absolutely have to? >>> >>> Middle ground may be harder, but may be nicer (especially since we are >>> talking about tightening behavior, making things that used to work now >>> fail is not nice if there is not a transition period where it first just >>> warns). >> >> We can do the nowadays usual thing: Add a -u option, print a deprecation >> warning if we don't have -u and can't open the backing file, and put >> it into the list of things to remove in 3.0. > > Works for me. > > Max > Sold, thanks for the idea. (Feel free to quabble on "unsafe" in the upcoming patch, I'm not sold on that either, but I wanted it to match the -u flag for rebase.) ((Maybe I'll keep it as -u but pretend it stands for --unchecked or some such nonsense.))
© 2016 - 2024 Red Hat, Inc.