| Message ID | 20170504193044.16303-1-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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
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
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(-)