* [Qemu-devel] [PATCH] qemu-img: Check for backing image if specified during create
@ 2017-05-04 19:30 John Snow
2017-05-04 19:58 ` Eric Blake
2017-05-04 20:00 ` no-reply
0 siblings, 2 replies; 4+ messages in thread
From: John Snow @ 2017-05-04 19:30 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, eblake, qemu-devel, mreitz, John Snow
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
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.
Inspired by: https://bugzilla.redhat.com/show_bug.cgi?id=1213786
Signed-off-by: John Snow <jsnow@redhat.com>
---
block.c | 75 +++++++++++++++++++++++-----------------------
qemu-img-cmds.hx | 4 +--
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) {
- 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(backing_options, "driver",
- qstring_from_str(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(backing_options, "driver",
+ qstring_from_str(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'",
@@ -4319,14 +4319,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 8ac7822..a3f82f3 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] [-o options] filename [size]")
+ "create [-q] [--object objectdef] [-f fmt] [-u] [-o options] filename [size]")
STEXI
-@item create [--object @var{objectdef}] [-q] [-f @var{fmt}] [-o @var{options}] @var{filename} [@var{size}]
+@item create [--object @var{objectdef}] [-q] [-f @var{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 bbe1574..2bfc422 100644
--- a/qemu-img.c
+++ 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"
" '-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 +410,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 +418,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 +466,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 +521,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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] qemu-img: Check for backing image if specified during create
2017-05-04 19:30 [Qemu-devel] [PATCH] qemu-img: Check for backing image if specified during create John Snow
@ 2017-05-04 19:58 ` Eric Blake
2017-05-04 21:02 ` John Snow
2017-05-04 20:00 ` no-reply
1 sibling, 1 reply; 4+ messages in thread
From: Eric Blake @ 2017-05-04 19:58 UTC (permalink / raw)
To: John Snow, qemu-block; +Cc: kwolf, qemu-devel, mreitz
[-- Attachment #1: Type: text/plain, Size: 4642 bytes --]
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 <jsnow@redhat.com>
> ---
> 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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] qemu-img: Check for backing image if specified during create
2017-05-04 19:30 [Qemu-devel] [PATCH] qemu-img: Check for backing image if specified during create John Snow
2017-05-04 19:58 ` Eric Blake
@ 2017-05-04 20:00 ` no-reply
1 sibling, 0 replies; 4+ messages in thread
From: no-reply @ 2017-05-04 20:00 UTC (permalink / raw)
To: jsnow; +Cc: famz, qemu-block, kwolf, qemu-devel, mreitz
Hi,
This series seems to have some coding style problems. See output below for
more information:
Subject: [Qemu-devel] [PATCH] qemu-img: Check for backing image if specified during create
Message-id: 20170504193044.16303-1-jsnow@redhat.com
Type: series
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
183b784 qemu-img: Check for backing image if specified during create
=== OUTPUT BEGIN ===
Checking PATCH 1/1: qemu-img: Check for backing image if specified during create...
ERROR: unnecessary whitespace before a quoted newline
#146: FILE: qemu-img.c:148:
+ " '-u' allows unsafe backing chains. For rebasing, it is assumed that old and new \n"
ERROR: unnecessary whitespace before a quoted newline
#147: FILE: qemu-img.c:149:
+ " backing file match exactly. The image doesn't need a working backing file \n"
total: 2 errors, 0 warnings, 166 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] qemu-img: Check for backing image if specified during create
2017-05-04 19:58 ` Eric Blake
@ 2017-05-04 21:02 ` John Snow
0 siblings, 0 replies; 4+ messages in thread
From: John Snow @ 2017-05-04 21:02 UTC (permalink / raw)
To: Eric Blake, qemu-block; +Cc: kwolf, qemu-devel, mreitz
On 05/04/2017 03:58 PM, Eric Blake wrote:
> 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/
>
That makes sense!
>> 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 <jsnow@redhat.com>
>> ---
>> 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.
>
OK, not a problem.
>> 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.
>
It's certainly not ideal.
>> +++ 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/
>
Yeah, this is a little messy, so I am relying on feedback on the
documentation.
>> +++ 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.
>
Sure. Only concern I have is I am not actually sure if passing
BDRV_O_NO_BACKING through has any other effect -- I don't THINK it does,
but there's a lot of things I think sometimes.
Thanks,
--js
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-05-04 21:02 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-05-04 19:30 [Qemu-devel] [PATCH] qemu-img: Check for backing image if specified during create John Snow
2017-05-04 19:58 ` Eric Blake
2017-05-04 21:02 ` John Snow
2017-05-04 20:00 ` no-reply
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).