Re: [Qemu-block] [Qemu-devel] [RFC PATCH 00/12] qemu-img: add bitmap queries
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20180512012537.22478-1-js...@redhat.com
Subject: [Qemu-devel] [RFC PATCH 00/12] qemu-img: add bitmap queries
=== 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
git config --local diff.algorithm histogram
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
From https://github.com/patchew-project/qemu
* [new tag] patchew/20180512012537.22478-1-js...@redhat.com ->
patchew/20180512012537.22478-1-js...@redhat.com
Switched to a new branch 'test'
98c78cde32 qemu-img: add bitmap clear
64b46d376f qemu-img: add bitmap dump
e820e6d20d qemu-img: split off common chunk of map command
15894bec9b qapi/block-core: add BitmapMapping and BitmapEntry structs
a09d1c3a65 qjson: allow caller to ask for arbitrary indent
384e244766 qcow2-bitmap: add basic bitmaps info
35cf9ace22 qapi: add bitmap info
36be89af25 qcow2-bitmap: track bitmap type
d347564ea3 qcow2/dirty-bitmaps: load IN_USE bitmaps if disk is RO
c554bff42b block/qcow2-bitmap: avoid adjusting bm->flags for RO bitmaps
9d00c36fba qcow2/dirty-bitmap: cache loaded bitmaps
7f34c78783 qcow2-bitmap: cache bm_list
=== OUTPUT BEGIN ===
Checking PATCH 1/12: qcow2-bitmap: cache bm_list...
Checking PATCH 2/12: qcow2/dirty-bitmap: cache loaded bitmaps...
Checking PATCH 3/12: block/qcow2-bitmap: avoid adjusting bm->flags for RO
bitmaps...
Checking PATCH 4/12: qcow2/dirty-bitmaps: load IN_USE bitmaps if disk is RO...
Checking PATCH 5/12: qcow2-bitmap: track bitmap type...
Checking PATCH 6/12: qapi: add bitmap info...
Checking PATCH 7/12: qcow2-bitmap: add basic bitmaps info...
WARNING: line over 80 characters
#43: FILE: block/qcow2-bitmap.c:72:
+[BME_FLAG_BIT_EXTRA] = { BME_FLAG_EXTRA,
BITMAP_FLAG_ENUM_EXTRA_DATA_COMPATIBLE },
total: 0 errors, 1 warnings, 118 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.
Checking PATCH 8/12: qjson: allow caller to ask for arbitrary indent...
Checking PATCH 9/12: qapi/block-core: add BitmapMapping and BitmapEntry
structs...
Checking PATCH 10/12: qemu-img: split off common chunk of map command...
WARNING: line over 80 characters
#74: FILE: qemu-img.c:2781:
+error_report("--output must be used with human or json as
argument.");
ERROR: open brace '{' following function declarations go on the next line
#127: FILE: qemu-img.c:2816:
+static void parse_unexpected(int argc, char *argv[]) {
total: 1 errors, 1 warnings, 173 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.
Checking PATCH 11/12: qemu-img: add bitmap dump...
ERROR: space prohibited after that '*' (ctx:BxW)
#179: FILE: qemu-img.c:3029:
+int (* handler)(BlockDriverState *b, BitmapOpts *opts);
^
total: 1 errors, 0 warnings, 203 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.
Checking PATCH 12/12: qemu-img: add bitmap clear...
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@redhat.com
[Qemu-block] [RFC PATCH 11/12] qemu-img: add bitmap dump
Signed-off-by: John Snow
---
qemu-img-cmds.hx | 6 ++
qemu-img.c | 185 +++
2 files changed, 191 insertions(+)
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 2fe31893cf..d25f359f5a 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -22,6 +22,12 @@ STEXI
@item bench [-c @var{count}] [-d @var{depth}] [-f @var{fmt}]
[--flush-interval=@var{flush_interval}] [-n] [--no-drain] [-o @var{offset}]
[--pattern=@var{pattern}] [-q] [-s @var{buffer_size}] [-S @var{step_size}] [-t
@var{cache}] [-w] [-U] @var{filename}
ETEXI
+DEF("bitmap", img_bitmap,
+"bitmap dump [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt]
[-U] filename [bitmap]")
+STEXI
+@item bitmap dump [--object @var{objectdef}] [--image-opts] [-f @var{fmt}]
[--output=@var{ofmt}] [-U] @var{filename} [@var{bitmap}]
+ETEXI
+
DEF("check", img_check,
"check [-q] [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt]
[-r [leaks | all]] [-T src_cache] [-U] filename")
STEXI
diff --git a/qemu-img.c b/qemu-img.c
index e31e38f674..1141216617 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2881,6 +2881,191 @@ out1:
return ret < 0;
}
+static BitmapMapping *get_bitmap_mapping(BdrvDirtyBitmap *bitmap)
+{
+BdrvDirtyBitmapIter *it = bdrv_dirty_iter_new(bitmap);
+BitmapMapping *bm = g_new0(BitmapMapping, 1);
+BitmapEntry *entry;
+BitmapEntryList *tmp, **last;
+int64_t offset;
+int64_t zero_offset;
+
+bm->name = g_strdup(bdrv_dirty_bitmap_name(bitmap));
+bm->entries = NULL;
+last = &bm->entries;
+
+while (true) {
+offset = bdrv_dirty_iter_next(it);
+if (offset == -1) {
+break;
+}
+
+zero_offset = bdrv_dirty_bitmap_next_zero(bitmap, offset);
+if (zero_offset == -1) {
+zero_offset = bdrv_dirty_bitmap_size(bitmap);
+}
+
+entry = g_new0(BitmapEntry, 1);
+entry->offset = offset;
+entry->length = zero_offset - offset;
+
+tmp = g_new0(BitmapEntryList, 1);
+tmp->value = entry;
+*last = tmp;
+last = &tmp->next;
+
+if (zero_offset >= bdrv_dirty_bitmap_size(bitmap)) {
+break;
+}
+bdrv_set_dirty_iter(it, zero_offset);
+}
+
+bdrv_dirty_iter_free(it);
+return bm;
+}
+
+static void dump_bitmap_mapping_json(BitmapMapping *bm, int indent,
+ FILE *stream)
+{
+QString *str;
+QObject *obj;
+Visitor *v = qobject_output_visitor_new(&obj);
+const int pretty = 1;
+
+visit_type_BitmapMapping(v, NULL, &bm, &error_abort);
+visit_complete(v, &obj);
+str = qobject_to_json_opt(obj, pretty, indent);
+assert(str != NULL);
+fprintf(stream, "%*s%s", 4 * indent, "", qstring_get_str(str));
+qobject_unref(obj);
+visit_free(v);
+qobject_unref(str);
+}
+
+static void dump_bitmap_mapping_human(BitmapMapping *bm, FILE *stream)
+{
+BitmapEntryList *bl = bm->entries;
+
+fprintf(stream, "# %s\n", bm->name);
+fprintf(stream, "%-16s%-16s\n", "Offset", "Length");
+for ( ; bl; bl = bl->next) {
+BitmapEntry *be = bl->value;
+fprintf(stream, "%-16"PRId64"%-16"PRId64"\n", be->offset, be->length);
+}
+}
+
+static void dump_bitmap_mapping(BitmapMapping *bm, OutputFormat output_format,
+int indent, FILE *stream)
+{
+switch (output_format) {
+case OFORMAT_JSON:
+dump_bitmap_mapping_json(bm, indent, stream);
+return;
+case OFORMAT_HUMAN:
+dump_bitmap_mapping_human(bm, stream);
+return;
+}
+}
+
+/* img_bitmap subcommands: dump */
+
+typedef struct BitmapOpts {
+CommonOpts base;
+const char *name;
+uint32_t granularity;
+} BitmapOpts;
+
+static int bitmap_cmd_dump(BlockDriverState *bs, BitmapOpts *opts)
+{
+BdrvDirtyBitmap *bitmap;
+BitmapMapping *bm;
+bool printed;
+OutputFormat ofmt = opts->base.output_format;
+
+if (opts->name) {
+bitmap = bdrv_find_dirty_bitmap(bs, opts->name);
+if (bitmap) {
+bm = get_bitmap_mapping(bitmap);
+dump_bitmap_mapping(bm, ofmt, 0, stdout);
+qapi_free_BitmapMapping(bm);
+} else {
+if (ofmt == OFORMAT_HUMAN) {
+fprintf(stdout, "Bitmap '%s' not found\n", opts->name);
+}
+}
+} else {
+if (ofmt == OFORMAT_JSON) {
+fprintf(stdout, "[\n");
+}
+for (bitmap = NULL, printed = false;
+ (bitmap = bdrv_dirty_bitmap_next(bs, bitmap)); printed = true) {
+if (printed) {
+fprintf(stdout, ofmt == OFORMAT_JSON ? ",\n" : "\n");
+}
+bm = get_bitmap_mapping(bitmap);
+dump_bitmap_mapping(bm, ofmt, 1, stdout);
+qapi_free_BitmapMapping(bm);
+}
+if (ofmt == OFORMAT_JSON) {
+fprintf(stdout, "%s%s", printed ? "\n" :
[Qemu-block] [RFC PATCH 12/12] qemu-img: add bitmap clear
Signed-off-by: John Snow
---
qemu-img-cmds.hx | 4 ++--
qemu-img.c | 46 --
2 files changed, 46 insertions(+), 4 deletions(-)
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index d25f359f5a..7b6ec73488 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -23,9 +23,9 @@ STEXI
ETEXI
DEF("bitmap", img_bitmap,
-"bitmap dump [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt]
[-U] filename [bitmap]")
+"bitmap [--object objectdef] [--image-opts] [-f fmt]
[--output=ofmt] [-U] filename [bitmap]")
STEXI
-@item bitmap dump [--object @var{objectdef}] [--image-opts] [-f @var{fmt}]
[--output=@var{ofmt}] [-U] @var{filename} [@var{bitmap}]
+@item bitmap [--object @var{objectdef}] [--image-opts] [-f
@var{fmt}] [--output=@var{ofmt}] [-U] @var{filename} [@var{bitmap}]
ETEXI
DEF("check", img_check,
diff --git a/qemu-img.c b/qemu-img.c
index 1141216617..b5ab8a3837 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2967,7 +2967,22 @@ static void dump_bitmap_mapping(BitmapMapping *bm,
OutputFormat output_format,
}
}
-/* img_bitmap subcommands: dump */
+static int do_bitmap_clear(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
+{
+bool enabled = bdrv_dirty_bitmap_enabled(bitmap);
+
+if (!enabled) {
+bdrv_enable_dirty_bitmap(bitmap);
+}
+bdrv_clear_dirty_bitmap(bitmap, NULL);
+if (!enabled) {
+bdrv_disable_dirty_bitmap(bitmap);
+}
+
+return 0;
+}
+
+/* img_bitmap subcommands: dump, clear */
typedef struct BitmapOpts {
CommonOpts base;
@@ -3017,6 +3032,30 @@ static int bitmap_cmd_dump(BlockDriverState *bs,
BitmapOpts *opts)
return 0;
}
+static int bitmap_cmd_clear(BlockDriverState *bs, BitmapOpts *opts)
+{
+BdrvDirtyBitmap *bitmap = NULL;
+
+if (opts->name) {
+bitmap = bdrv_find_dirty_bitmap(bs, opts->name);
+if (!bitmap) {
+error_report("No bitmap named '%s' found", opts->name);
+return -1;
+}
+if (do_bitmap_clear(bs, bitmap)) {
+return -1;
+}
+} else {
+while ((bitmap = bdrv_dirty_bitmap_next(bs, bitmap))) {
+if (do_bitmap_clear(bs, bitmap)) {
+return -1;
+}
+}
+}
+
+return 0;
+}
+
static int img_bitmap(int argc, char **argv)
{
const char *cmd;
@@ -3029,12 +3068,15 @@ static int img_bitmap(int argc, char **argv)
int (* handler)(BlockDriverState *b, BitmapOpts *opts);
if (argc < 2) {
-error_report("Expected a bitmap subcommand: ");
+error_report("Expected a bitmap subcommand: ");
return EXIT_FAILURE;
}
cmd = argv[1];
if (strcmp(cmd, "dump") == 0) {
handler = bitmap_cmd_dump;
+} else if (strcmp(cmd, "clear") == 0) {
+flags |= BDRV_O_RDWR;
+handler = bitmap_cmd_clear;
} else {
error_report("Unrecognized bitmap subcommand '%s'", cmd);
return EXIT_FAILURE;
--
2.14.3
[Qemu-block] [RFC PATCH 10/12] qemu-img: split off common chunk of map command
It will be re-used for a bitmap listing command.
Signed-off-by: John Snow
---
qemu-img.c | 192 +++--
1 file changed, 110 insertions(+), 82 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index ea62d2d61e..e31e38f674 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -69,8 +69,8 @@ enum {
};
typedef enum OutputFormat {
-OFORMAT_JSON,
OFORMAT_HUMAN,
+OFORMAT_JSON,
} OutputFormat;
/* Default to cache=writeback as data integrity is not important for qemu-img
*/
@@ -2728,96 +2728,122 @@ static inline bool entry_mergeable(const MapEntry
*curr, const MapEntry *next)
return true;
}
+typedef struct CommonOpts {
+OutputFormat output_format;
+const char *filename;
+const char *fmt;
+bool force_share;
+bool image_opts;
+} CommonOpts;
+
+static int parse_opts_common(CommonOpts *opts, int argc, char **argv)
+{
+int c;
+
+for (;;) {
+int option_index = 0;
+static const struct option long_options[] = {
+{"help", no_argument, 0, 'h'},
+{"format", required_argument, 0, 'f'},
+{"output", required_argument, 0, OPTION_OUTPUT},
+{"object", required_argument, 0, OPTION_OBJECT},
+{"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
+{"force-share", no_argument, 0, 'U'},
+{0, 0, 0, 0}
+};
+c = getopt_long(argc, argv, ":f:hU",
+long_options, &option_index);
+if (c == -1) {
+break;
+}
+switch (c) {
+case ':':
+missing_argument(argv[optind - 1]);
+break;
+case '?':
+unrecognized_option(argv[optind - 1]);
+break;
+case 'h':
+help();
+break;
+case 'f':
+opts->fmt = optarg;
+break;
+case 'U':
+opts->force_share = true;
+break;
+case OPTION_OUTPUT:
+if (optarg && !strcmp(optarg, "json")) {
+opts->output_format = OFORMAT_JSON;
+} else if (optarg && !strcmp(optarg, "human")) {
+opts->output_format = OFORMAT_HUMAN;
+} else {
+error_report("--output must be used with human or json as
argument.");
+return -EINVAL;
+}
+break;
+case OPTION_OBJECT: {
+QemuOpts *opts;
+opts = qemu_opts_parse_noisily(&qemu_object_opts,
+ optarg, true);
+if (!opts) {
+return -EINVAL;
+}
+} break;
+case OPTION_IMAGE_OPTS:
+opts->image_opts = true;
+break;
+}
+}
+if (qemu_opts_foreach(&qemu_object_opts,
+ user_creatable_add_opts_foreach,
+ NULL, NULL)) {
+return -EINVAL;
+}
+
+return 0;
+}
+
+static void parse_positional(int argc, char *argv[],
+ bool opt, const char **str, const char *name) {
+if (!opt && optind >= argc) {
+error_exit("Expecting '%s' positional parameter", name);
+} else if (optind < argc) {
+*str = argv[optind++];
+}
+}
+
+static void parse_unexpected(int argc, char *argv[]) {
+if (optind != argc) {
+error_exit("Unexpected argument '%s'\n", argv[optind]);
+}
+}
+
static int img_map(int argc, char **argv)
{
-int c;
-OutputFormat output_format = OFORMAT_HUMAN;
BlockBackend *blk;
BlockDriverState *bs;
-const char *filename, *fmt, *output;
int64_t length;
+int ret = 0;
MapEntry curr = { .length = 0 }, next;
-int ret = 0;
-bool image_opts = false;
-bool force_share = false;
+CommonOpts *opts;
-fmt = NULL;
-output = NULL;
-for (;;) {
-int option_index = 0;
-static const struct option long_options[] = {
-{"help", no_argument, 0, 'h'},
-{"format", required_argument, 0, 'f'},
-{"output", required_argument, 0, OPTION_OUTPUT},
-{"object", required_argument, 0, OPTION_OBJECT},
-{"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
-{"force-share", no_argument, 0, 'U'},
-{0, 0, 0, 0}
-};
-c = getopt_long(argc, argv, ":f:hU",
-long_options, &option_index);
-if (c == -1) {
-break;
-}
-switch (c) {
-case ':':
-missing_argument(argv[optind - 1]);
-break;
-case '?':
-unrecognized_option(argv[optind - 1]);
-break;
-case 'h':
-help();
-break;
-case 'f':
-fmt = optarg;
-break;
-case 'U':
-force_share = true;
-break;
-case OPTION_OUTPUT:
-output = optarg;
-break;
-
[Qemu-block] [RFC PATCH 06/12] qapi: add bitmap info
Add some of the necessary scaffolding for reporting bitmap information.
Signed-off-by: John Snow
---
qapi/block-core.json | 60 +++-
1 file changed, 59 insertions(+), 1 deletion(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index c50517bff3..8f33f41ce7 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -33,6 +33,61 @@
'date-sec': 'int', 'date-nsec': 'int',
'vm-clock-sec': 'int', 'vm-clock-nsec': 'int' } }
+##
+# @BitmapTypeEnum:
+#
+# An enumeration of possible serialized bitmap types.
+#
+# @dirty-tracking: This bitmap records information on dirty
+# segments within the file.
+#
+# @unknown: This bitmap is of an unknown or reserved type.
+#
+# Since: 2.13
+##
+{ 'enum': 'BitmapTypeEnum', 'data': [ 'dirty-tracking', 'unknown' ] }
+
+##
+# @BitmapFlagEnum:
+#
+# An enumeration of possible flags for serialized bitmaps.
+#
+# @in-use: This bitmap is considered to be in-use, and may now be inconsistent.
+#
+# @auto: This bitmap must reflect any and all changes to the file it describes.
+#The type of this bitmap must be @DirtyTrackingBitmap.
+#
+# @extra-data-compatible: This bitmap has extra information associated with it.
+#
+# @unknown: This bitmap has unknown or reserved properties.
+#
+# Since: 2.13
+##
+{ 'enum': 'BitmapFlagEnum', 'data': [ 'in-use', 'auto',
+ 'extra-data-compatible', 'unknown' ] }
+
+##
+# @BitmapInfo:
+#
+# @name: The name of the bitmap.
+#
+# @type: The type of bitmap.
+#
+# @granularity: Bitmap granularity, in bytes.
+#
+# @count: Overall bitmap dirtiness, in bytes.
+#
+# @flags: Bitmap flags, if any.
+#
+# Since: 2.13
+#
+##
+{ 'struct': 'BitmapInfo',
+ 'data': { 'name': 'str', 'type': 'BitmapTypeEnum', 'granularity': 'int',
+'count': 'int', '*flags': ['BitmapFlagEnum']
+ }
+}
+
##
# @ImageInfoSpecificQCow2EncryptionBase:
#
@@ -69,6 +124,8 @@
# @encrypt: details about encryption parameters; only set if image
# is encrypted (since 2.10)
#
+# @bitmaps: list of image bitmaps (since 2.13)
+#
# Since: 1.7
##
{ 'struct': 'ImageInfoSpecificQCow2',
@@ -77,7 +134,8 @@
'*lazy-refcounts': 'bool',
'*corrupt': 'bool',
'refcount-bits': 'int',
- '*encrypt': 'ImageInfoSpecificQCow2Encryption'
+ '*encrypt': 'ImageInfoSpecificQCow2Encryption',
+ '*bitmaps': ['BitmapInfo']
} }
##
--
2.14.3
[Qemu-block] [RFC PATCH 04/12] qcow2/dirty-bitmaps: load IN_USE bitmaps if disk is RO
For the purposes of qemu-img manipulation and querying of bitmaps, load
bitmaps that are "in use" -- if the image is read only. This will allow
us to diagnose problems with this flag using the qemu-img tool.
Signed-off-by: John Snow
---
block/qcow2-bitmap.c | 32 +++-
1 file changed, 15 insertions(+), 17 deletions(-)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index fc8d52fc3e..b556dbdccd 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -346,7 +346,7 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
uint32_t granularity;
BdrvDirtyBitmap *bitmap = NULL;
-if (bm->flags & BME_FLAG_IN_USE) {
+if (can_write(bs) && (bm->flags & BME_FLAG_IN_USE)) {
error_setg(errp, "Bitmap '%s' is in use", bm->name);
goto fail;
}
@@ -974,23 +974,21 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error
**errp)
}
QSIMPLEQ_FOREACH(bm, bm_list, entry) {
-if (!(bm->flags & BME_FLAG_IN_USE)) {
-BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
-if (bitmap == NULL) {
-goto fail;
-}
-bm->dirty_bitmap = bitmap;
+BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
+if (bitmap == NULL) {
+goto fail;
+}
+bm->dirty_bitmap = bitmap;
-if (!(bm->flags & BME_FLAG_AUTO)) {
-bdrv_disable_dirty_bitmap(bitmap);
-}
-bdrv_dirty_bitmap_set_persistance(bitmap, true);
-if (can_write(bs)) {
-bm->flags |= BME_FLAG_IN_USE;
-needs_update = true;
-} else {
-bdrv_dirty_bitmap_set_readonly(bitmap, true);
-}
+if (!(bm->flags & BME_FLAG_AUTO)) {
+bdrv_disable_dirty_bitmap(bitmap);
+}
+bdrv_dirty_bitmap_set_persistance(bitmap, true);
+if (can_write(bs)) {
+bm->flags |= BME_FLAG_IN_USE;
+needs_update = true;
+} else {
+bdrv_dirty_bitmap_set_readonly(bitmap, true);
}
}
--
2.14.3
[Qemu-block] [RFC PATCH 09/12] qapi/block-core: add BitmapMapping and BitmapEntry structs
Add two new structures for detailing the marked regions of bitmaps as
saved in e.g. qcow2 files.
Signed-off-by: John Snow
---
qapi/block-core.json | 32
1 file changed, 32 insertions(+)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 8f33f41ce7..de8ad73a78 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -298,6 +298,38 @@
'zero': 'bool', 'depth': 'int', '*offset': 'int',
'*filename': 'str' } }
+##
+# @BitmapEntry:
+#
+# Dirty Bitmap region information for a virtual block range
+#
+# @offset: the start byte of the dirty virtual range
+#
+# @length: the number of bytes of the dirty virtual range
+#
+# Since: 2.13
+#
+##
+{ 'struct': 'BitmapEntry',
+ 'data': { 'offset': 'int', 'length': 'int' } }
+
+##
+# @BitmapMapping:
+#
+# List of described regions correlated to a named bitmap.
+#
+# @name: The name of the bitmap whose range is described here
+#
+# @entries: A list of zero or more @BitmapEntry elements representing
+# the range(s) described by the bitmap.
+#
+# Since: 2.13
+#
+##
+{ 'struct': 'BitmapMapping',
+ 'data': { 'name': 'str',
+'entries': [ 'BitmapEntry' ] } }
+
##
# @BlockdevCacheInfo:
#
--
2.14.3
[Qemu-block] [RFC PATCH 02/12] qcow2/dirty-bitmap: cache loaded bitmaps
For bitmaps that we succeeded in loading, we can cache a reference
to that object. This will let us iterate over the more convenient
form of in-memory bitmaps for qemu-img bitmap manipulation tools.
Signed-off-by: John Snow
---
block/qcow2-bitmap.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index fb0a4f3ec4..d89758f235 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -986,6 +986,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error
**errp)
if (bitmap == NULL) {
goto fail;
}
+bm->dirty_bitmap = bitmap;
if (!(bm->flags & BME_FLAG_AUTO)) {
bdrv_disable_dirty_bitmap(bitmap);
--
2.14.3
[Qemu-block] [RFC PATCH 00/12] qemu-img: add bitmap queries
Allow qemu-img to show information about bitmaps stored in qcow2 images. Add a 'bitmap' meta-command with 'dump' sub-command to retrieve a list of dirty regions in bitmaps stored in a qcow2 image. RFC: - I am not 1000% convinced the bm_list caching is perfectly safe, especially with respect to migration and inactivation. There are more efficiencies and tweaks I can make, but I think this is the minimal set. - I decided not to gate bitmap info in the "info" command behind extra flags. - Bitmap data-gathering in "bitmap dump" could be made more space-efficient by just reporting one segment at a time, probably. - `bitmap rm` and `bitmap mk` need extra work, so I am not submitting them just yet: rm needs more work around the remove persistence API, and make needs more work around the "can add" API. - None of these commands will work with "in use" bitmaps; we need qemu-img check -r bitmaps support for this. I'm not sure what the right behavior to "fix" in-use bitmaps should be: - Cleared: This might be dangerous. - Fully Set: This is safer, but stupid. - Deleted: This might be the best option. - Un-set in-use: VERY dangerous; would rather not. John Snow (12): qcow2-bitmap: cache bm_list qcow2/dirty-bitmap: cache loaded bitmaps block/qcow2-bitmap: avoid adjusting bm->flags for RO bitmaps qcow2/dirty-bitmaps: load IN_USE bitmaps if disk is RO qcow2-bitmap: track bitmap type qapi: add bitmap info qcow2-bitmap: add basic bitmaps info qjson: allow caller to ask for arbitrary indent qapi/block-core: add BitmapMapping and BitmapEntry structs qemu-img: split off common chunk of map command qemu-img: add bitmap dump qemu-img: add bitmap clear block/qcow2-bitmap.c | 220 + block/qcow2.c| 9 + block/qcow2.h| 3 + include/qapi/qmp/qjson.h | 1 + qapi/block-core.json | 92 ++- qemu-img-cmds.hx | 6 + qemu-img.c | 419 +-- qobject/qjson.c | 21 +-- 8 files changed, 612 insertions(+), 159 deletions(-) -- 2.14.3
[Qemu-block] [RFC PATCH 01/12] qcow2-bitmap: cache bm_list
We don't need to re-read this list every time, exactly. We can keep it cached
and delete our copy when we flush to disk.
Because we don't try to flush bitmaps on close if there's nothing to flush,
add a new conditional to delete the state anyway for a clean exit.
Signed-off-by: John Snow
---
block/qcow2-bitmap.c | 74
block/qcow2.c| 2 ++
block/qcow2.h| 2 ++
3 files changed, 50 insertions(+), 28 deletions(-)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 6e93ec43e1..fb0a4f3ec4 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -536,8 +536,7 @@ static uint32_t bitmap_list_count(Qcow2BitmapList *bm_list)
* Get bitmap list from qcow2 image. Actually reads bitmap directory,
* checks it and convert to bitmap list.
*/
-static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, uint64_t offset,
- uint64_t size, Error **errp)
+static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, Error **errp)
{
int ret;
BDRVQcow2State *s = bs->opaque;
@@ -545,6 +544,8 @@ static Qcow2BitmapList *bitmap_list_load(BlockDriverState
*bs, uint64_t offset,
Qcow2BitmapDirEntry *e;
uint32_t nb_dir_entries = 0;
Qcow2BitmapList *bm_list = NULL;
+uint64_t offset = s->bitmap_directory_offset;
+uint64_t size = s->bitmap_directory_size;
if (size == 0) {
error_setg(errp, "Requested bitmap directory size is zero");
@@ -636,6 +637,30 @@ fail:
return NULL;
}
+static Qcow2BitmapList *get_bitmap_list(BlockDriverState *bs, Error **errp)
+{
+BDRVQcow2State *s = bs->opaque;
+Qcow2BitmapList *bm_list;
+
+if (s->bitmap_list) {
+return (Qcow2BitmapList *)s->bitmap_list;
+}
+
+bm_list = bitmap_list_load(bs, errp);
+s->bitmap_list = bm_list;
+return bm_list;
+}
+
+static void del_bitmap_list(BlockDriverState *bs)
+{
+BDRVQcow2State *s = bs->opaque;
+
+if (s->bitmap_list) {
+bitmap_list_free(s->bitmap_list);
+s->bitmap_list = NULL;
+}
+}
+
int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
void **refcount_table,
int64_t *refcount_table_size)
@@ -656,8 +681,7 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs,
BdrvCheckResult *res,
return ret;
}
-bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
- s->bitmap_directory_size, NULL);
+bm_list = get_bitmap_list(bs, NULL);
if (bm_list == NULL) {
res->corruptions++;
return -EINVAL;
@@ -707,8 +731,6 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs,
BdrvCheckResult *res,
}
out:
-bitmap_list_free(bm_list);
-
return ret;
}
@@ -953,8 +975,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error
**errp)
return false;
}
-bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
- s->bitmap_directory_size, errp);
+bm_list = get_bitmap_list(bs, errp);
if (bm_list == NULL) {
return false;
}
@@ -992,14 +1013,12 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs,
Error **errp)
}
g_slist_free(created_dirty_bitmaps);
-bitmap_list_free(bm_list);
-
return header_updated;
fail:
g_slist_foreach(created_dirty_bitmaps, release_dirty_bitmap_helper, bs);
g_slist_free(created_dirty_bitmaps);
-bitmap_list_free(bm_list);
+del_bitmap_list(bs);
return false;
}
@@ -1027,8 +1046,7 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs,
bool *header_updated,
return -EINVAL;
}
-bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
- s->bitmap_directory_size, errp);
+bm_list = get_bitmap_list(bs, errp);
if (bm_list == NULL) {
return -EINVAL;
}
@@ -1068,7 +1086,6 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs,
bool *header_updated,
out:
g_slist_free(ro_dirty_bitmaps);
-bitmap_list_free(bm_list);
return ret;
}
@@ -1277,8 +1294,7 @@ void
qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
return;
}
-bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
- s->bitmap_directory_size, errp);
+bm_list = get_bitmap_list(bs, errp);
if (bm_list == NULL) {
return;
}
@@ -1300,7 +1316,11 @@ void
qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
fail:
bitmap_free(bm);
-bitmap_list_free(bm_list);
+}
+
+void qcow2_persistent_dirty_bitmaps_cache_destroy(BlockDriverState *bs)
+{
+del_bitmap_list(bs);
}
void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
@@ -1317,12 +1337,12 @@ void
qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
if (!bdrv_has_changed_per
[Qemu-block] [RFC PATCH 07/12] qcow2-bitmap: add basic bitmaps info
Add functions for querying the basic information inside of bitmaps.
Restructure the bitmaps flags masks to facilitate providing a list of
flags belonging to the bitmap(s) being queried.
Signed-off-by: John Snow
---
block/qcow2-bitmap.c | 81 ++--
block/qcow2.c| 7 +
block/qcow2.h| 1 +
3 files changed, 87 insertions(+), 2 deletions(-)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 60e01abfd7..811b82743a 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -49,8 +49,28 @@
/* Bitmap directory entry flags */
#define BME_RESERVED_FLAGS 0xfffcU
-#define BME_FLAG_IN_USE (1U << 0)
-#define BME_FLAG_AUTO (1U << 1)
+
+enum BME_FLAG_BITS {
+BME_FLAG_BIT__BEGIN = 0,
+BME_FLAG_BIT_IN_USE = BME_FLAG_BIT__BEGIN,
+BME_FLAG_BIT_AUTO = 1,
+BME_FLAG_BIT_EXTRA = 2,
+BME_FLAG_BIT__MAX,
+};
+
+#define BME_FLAG_IN_USE (1U << BME_FLAG_BIT_IN_USE)
+#define BME_FLAG_AUTO (1U << BME_FLAG_BIT_AUTO)
+#define BME_FLAG_EXTRA (1U << BME_FLAG_BIT_EXTRA)
+
+/* Correlate canonical spec values to autogenerated QAPI values */
+struct {
+uint32_t mask;
+int qapi_value;
+} BMEFlagMap[BME_FLAG_BIT__MAX] = {
+[BME_FLAG_BIT_IN_USE] = { BME_FLAG_IN_USE, BITMAP_FLAG_ENUM_IN_USE },
+[BME_FLAG_BIT_AUTO] = { BME_FLAG_AUTO, BITMAP_FLAG_ENUM_AUTO },
+[BME_FLAG_BIT_EXTRA] = { BME_FLAG_EXTRA,
BITMAP_FLAG_ENUM_EXTRA_DATA_COMPATIBLE },
+};
/* bits [1, 8] U [56, 63] are reserved */
#define BME_TABLE_ENTRY_RESERVED_MASK 0xff0001feULL
@@ -663,6 +683,63 @@ static void del_bitmap_list(BlockDriverState *bs)
}
}
+static BitmapFlagEnumList *get_bitmap_flags(uint32_t flags)
+{
+int i;
+BitmapFlagEnumList *flist = NULL;
+BitmapFlagEnumList *ftmp;
+
+while (flags) {
+i = ctz32(flags);
+ftmp = g_new0(BitmapFlagEnumList, 1);
+if (i >= BME_FLAG_BIT__BEGIN && i < BME_FLAG_BIT__MAX) {
+ftmp->value = BMEFlagMap[i].qapi_value;
+} else {
+ftmp->value = BITMAP_FLAG_ENUM_UNKNOWN;
+}
+ftmp->next = flist;
+flist = ftmp;
+flags &= ~(1 << i);
+}
+
+return flist;
+}
+
+BitmapInfoList *qcow2_get_bitmap_info(BlockDriverState *bs)
+{
+Qcow2BitmapList *bm_list;
+Qcow2Bitmap *bm;
+BitmapInfoList *info_list = NULL;
+BitmapInfoList *tmp;
+BitmapInfo *bitmap_info;
+
+bm_list = get_bitmap_list(bs, NULL);
+if (!bm_list) {
+return info_list;
+}
+
+QSIMPLEQ_FOREACH(bm, bm_list, entry) {
+bitmap_info = g_new0(BitmapInfo, 1);
+bitmap_info->name = g_strdup(bm->name);
+if (bm->type == BT_DIRTY_TRACKING_BITMAP) {
+bitmap_info->type = BITMAP_TYPE_ENUM_DIRTY_TRACKING;
+} else {
+bitmap_info->type = BITMAP_TYPE_ENUM_UNKNOWN;
+}
+bitmap_info->granularity = 1 << bm->granularity_bits;
+bitmap_info->count = bdrv_get_dirty_count(bm->dirty_bitmap);
+bitmap_info->flags = get_bitmap_flags(bm->flags);
+bitmap_info->has_flags = !!bitmap_info->flags;
+
+tmp = g_new0(BitmapInfoList, 1);
+tmp->value = bitmap_info;
+tmp->next = info_list;
+info_list = tmp;
+}
+
+return info_list;
+}
+
int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
void **refcount_table,
int64_t *refcount_table_size)
diff --git a/block/qcow2.c b/block/qcow2.c
index 7ae9000656..0b24ecb6b2 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3960,6 +3960,7 @@ static ImageInfoSpecific
*qcow2_get_specific_info(BlockDriverState *bs)
BDRVQcow2State *s = bs->opaque;
ImageInfoSpecific *spec_info;
QCryptoBlockInfo *encrypt_info = NULL;
+BitmapInfoList *bitmap_list = NULL;
if (s->crypto != NULL) {
encrypt_info = qcrypto_block_get_info(s->crypto, &error_abort);
@@ -4016,6 +4017,12 @@ static ImageInfoSpecific
*qcow2_get_specific_info(BlockDriverState *bs)
spec_info->u.qcow2.data->encrypt = qencrypt;
}
+bitmap_list = qcow2_get_bitmap_info(bs);
+if (bitmap_list) {
+spec_info->u.qcow2.data->has_bitmaps = true;
+spec_info->u.qcow2.data->bitmaps = bitmap_list;
+}
+
return spec_info;
}
diff --git a/block/qcow2.h b/block/qcow2.h
index 796a8c914b..76bf5fa55d 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -686,5 +686,6 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
const char *name,
Error **errp);
+BitmapInfoList *qcow2_get_bitmap_info(BlockDriverState *bs);
#endif
--
2.14.3
[Qemu-block] [RFC PATCH 05/12] qcow2-bitmap: track bitmap type
We only have one type of persistent bitmap right now, but I'd like the
qemu-img tool to be able to give good diagnostic information if it sees
an unknown/unsupported type.
We do enforce it to be the dirty tracking type in check_dir_entry, but
I wanted positive affirmation of the type in the forthcoming info script.
Signed-off-by: John Snow
---
block/qcow2-bitmap.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index b556dbdccd..60e01abfd7 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -83,6 +83,7 @@ typedef QSIMPLEQ_HEAD(Qcow2BitmapTableList, Qcow2BitmapTable)
typedef struct Qcow2Bitmap {
Qcow2BitmapTable table;
uint32_t flags;
+uint8_t type;
uint8_t granularity_bits;
char *name;
@@ -608,6 +609,7 @@ static Qcow2BitmapList *bitmap_list_load(BlockDriverState
*bs, Error **errp)
bm->table.offset = e->bitmap_table_offset;
bm->table.size = e->bitmap_table_size;
bm->flags = e->flags;
+bm->type = e->type;
bm->granularity_bits = e->granularity_bits;
bm->name = dir_entry_copy_name(e);
QSIMPLEQ_INSERT_TAIL(bm_list, bm, entry);
--
2.14.3
[Qemu-block] [RFC PATCH 08/12] qjson: allow caller to ask for arbitrary indent
The function already exists, just expose it.
Signed-off-by: John Snow
---
include/qapi/qmp/qjson.h | 1 +
qobject/qjson.c | 21 +++--
2 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/include/qapi/qmp/qjson.h b/include/qapi/qmp/qjson.h
index b274ac3a86..0e8624c1fa 100644
--- a/include/qapi/qmp/qjson.h
+++ b/include/qapi/qmp/qjson.h
@@ -19,6 +19,7 @@ QObject *qobject_from_jsonf(const char *string, ...)
GCC_FMT_ATTR(1, 2);
QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
GCC_FMT_ATTR(1, 0);
+QString *qobject_to_json_opt(const QObject *obj, int pretty, int indent);
QString *qobject_to_json(const QObject *obj);
QString *qobject_to_json_pretty(const QObject *obj);
diff --git a/qobject/qjson.c b/qobject/qjson.c
index 9816a65c7d..d603e1cce1 100644
--- a/qobject/qjson.c
+++ b/qobject/qjson.c
@@ -252,20 +252,21 @@ static void to_json(const QObject *obj, QString *str, int
pretty, int indent)
}
}
+QString *qobject_to_json_opt(const QObject *obj, int pretty, int indent)
+{
+QString *str = qstring_new();
+
+to_json(obj, str, pretty, indent);
+
+return str;
+}
+
QString *qobject_to_json(const QObject *obj)
{
-QString *str = qstring_new();
-
-to_json(obj, str, 0, 0);
-
-return str;
+return qobject_to_json_opt(obj, 0, 0);
}
QString *qobject_to_json_pretty(const QObject *obj)
{
-QString *str = qstring_new();
-
-to_json(obj, str, 1, 0);
-
-return str;
+return qobject_to_json_opt(obj, 1, 0);
}
--
2.14.3
[Qemu-block] [RFC PATCH 03/12] block/qcow2-bitmap: avoid adjusting bm->flags for RO bitmaps
Instead of always setting IN_USE, handle whether or not the bitmap
is read-only instead of a two-loop pass. This will allow us to show
the flags exactly as they appear for bitmaps in `qemu-img info`.
Signed-off-by: John Snow
---
block/qcow2-bitmap.c | 48
1 file changed, 20 insertions(+), 28 deletions(-)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index d89758f235..fc8d52fc3e 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -942,13 +942,6 @@ fail:
return ret;
}
-/* for g_slist_foreach for GSList of BdrvDirtyBitmap* elements */
-static void release_dirty_bitmap_helper(gpointer bitmap,
-gpointer bs)
-{
-bdrv_release_dirty_bitmap(bs, bitmap);
-}
-
/* for g_slist_foreach for GSList of BdrvDirtyBitmap* elements */
static void set_readonly_helper(gpointer bitmap, gpointer value)
{
@@ -967,8 +960,8 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error
**errp)
BDRVQcow2State *s = bs->opaque;
Qcow2BitmapList *bm_list;
Qcow2Bitmap *bm;
-GSList *created_dirty_bitmaps = NULL;
bool header_updated = false;
+bool needs_update = false;
if (s->nb_bitmaps == 0) {
/* No bitmaps - nothing to do */
@@ -992,33 +985,32 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error
**errp)
bdrv_disable_dirty_bitmap(bitmap);
}
bdrv_dirty_bitmap_set_persistance(bitmap, true);
-bm->flags |= BME_FLAG_IN_USE;
-created_dirty_bitmaps =
-g_slist_append(created_dirty_bitmaps, bitmap);
-}
-}
-
-if (created_dirty_bitmaps != NULL) {
-if (can_write(bs)) {
-/* in_use flags must be updated */
-int ret = update_ext_header_and_dir_in_place(bs, bm_list);
-if (ret < 0) {
-error_setg_errno(errp, -ret, "Can't update bitmap directory");
-goto fail;
+if (can_write(bs)) {
+bm->flags |= BME_FLAG_IN_USE;
+needs_update = true;
+} else {
+bdrv_dirty_bitmap_set_readonly(bitmap, true);
}
-header_updated = true;
-} else {
-g_slist_foreach(created_dirty_bitmaps, set_readonly_helper,
-(gpointer)true);
}
}
-g_slist_free(created_dirty_bitmaps);
+/* in_use flags must be updated */
+if (needs_update) {
+int ret = update_ext_header_and_dir_in_place(bs, bm_list);
+if (ret < 0) {
+error_setg_errno(errp, -ret, "Can't update bitmap directory");
+goto fail;
+}
+header_updated = true;
+}
return header_updated;
fail:
-g_slist_foreach(created_dirty_bitmaps, release_dirty_bitmap_helper, bs);
-g_slist_free(created_dirty_bitmaps);
+QSIMPLEQ_FOREACH(bm, bm_list, entry) {
+if (bm->dirty_bitmap) {
+bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
+}
+}
del_bitmap_list(bs);
return false;
--
2.14.3
Re: [Qemu-block] [PATCH 11/42] job: Add job_delete()
On 2018-05-09 18:26, Kevin Wolf wrote: > This moves freeing the Job object and its fields from block_job_unref() > to job_delete(). > > Signed-off-by: Kevin Wolf > --- > include/qemu/job.h | 3 +++ > blockjob.c | 3 +-- > job.c | 6 ++ > 3 files changed, 10 insertions(+), 2 deletions(-) Reviewed-by: Max Reitz signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH 10/42] job: Add JobDriver.job_type
On 2018-05-09 18:26, Kevin Wolf wrote:
> This moves the job_type field from BlockJobDriver to JobDriver.
>
> Signed-off-by: Kevin Wolf
> ---
> include/block/blockjob_int.h | 3 ---
> include/qemu/job.h | 11 +++
> block/backup.c | 2 +-
> block/commit.c | 2 +-
> block/mirror.c | 4 ++--
> block/stream.c | 2 +-
> blockjob.c | 16 +++-
> job.c| 10 ++
> 8 files changed, 33 insertions(+), 17 deletions(-)
>
[...]
> diff --git a/include/qemu/job.h b/include/qemu/job.h
> index b4b49f19e1..c87e951c8a 100644
> --- a/include/qemu/job.h
> +++ b/include/qemu/job.h
[...]
> @@ -57,4 +62,10 @@ struct JobDriver {
> */
> void *job_create(const char *job_id, const JobDriver *driver, Error **errp);
>
> +/** Returns the JobType of a given Job. */
> +JobType job_type(Job *job);
> +
> +/** Returns the enum string for the JobType of a given Job. */
> +const char *job_type_str(Job *job);
> +
Is there a good reason for these not to take a const Job *?
Depending on the answer:
Reviewed-by: Max Reitz
> #endif
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH 09/42] job: Rename BlockJobType into JobType
On 2018-05-09 18:26, Kevin Wolf wrote: > QAPI types aren't externally visible, so we can rename them without > causing problems. Before we add a job type to Job, rename the enum > so it can be used for more than just block jobs. > > Signed-off-by: Kevin Wolf > Reviewed-by: Eric Blake > --- > qapi/block-core.json | 14 +++--- > include/block/blockjob_int.h | 2 +- > block/backup.c | 2 +- > block/commit.c | 2 +- > block/mirror.c | 4 ++-- > block/stream.c | 2 +- > blockjob.c | 6 +++--- > 7 files changed, 16 insertions(+), 16 deletions(-) Reviewed-by: Max Reitz signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH 08/42] job: Create Job, JobDriver and job_create()
On 2018-05-09 18:26, Kevin Wolf wrote:
> This is the first step towards creating an infrastructure for generic
> background jobs that aren't tied to a block device. For now, Job only
> stores its ID and JobDriver, the rest stays in BlockJob.
>
> The following patches will move over more parts of BlockJob to Job if
> they are meaningful outside the context of a block job.
>
> Signed-off-by: Kevin Wolf
> ---
> include/block/blockjob.h | 9 +++
> include/block/blockjob_int.h | 4 +--
> include/qemu/job.h | 60
>
> block/backup.c | 4 ++-
> block/commit.c | 4 ++-
> block/mirror.c | 10 +---
> block/stream.c | 4 ++-
> blockjob.c | 46 -
> job.c| 48 +++
> tests/test-bdrv-drain.c | 4 ++-
> tests/test-blockjob-txn.c| 4 ++-
> tests/test-blockjob.c| 12 ++---
> MAINTAINERS | 2 ++
> Makefile.objs| 2 +-
> 14 files changed, 169 insertions(+), 44 deletions(-)
> create mode 100644 include/qemu/job.h
> create mode 100644 job.c
>
> diff --git a/include/block/blockjob.h b/include/block/blockjob.h
> index 0b57d53084..8acc1a236a 100644
> --- a/include/block/blockjob.h
> +++ b/include/block/blockjob.h
[...]
> @@ -40,6 +41,9 @@ typedef struct BlockJobTxn BlockJobTxn;
> * Long-running operation on a BlockDriverState.
> */
> typedef struct BlockJob {
> +/** Data belonging to the generic Job infrastructure */
> +Job job;
> +
> /** The job type, including the job vtable. */
> const BlockJobDriver *driver;
Any reason why you keep this field around? Shouldn't it be just
DO_UPCAST(const BlockJobDriver, job_driver, job.driver)?
>
> @@ -47,11 +51,6 @@ typedef struct BlockJob {
> BlockBackend *blk;
>
> /**
> - * The ID of the block job. May be NULL for internal jobs.
> - */
> -char *id;
> -
> -/**
> * The coroutine that executes the job. If not NULL, it is
> * reentered when busy is false and the job is cancelled.
> */
[...]
> diff --git a/blockjob.c b/blockjob.c
> index 9943218a34..35d604d05a 100644
> --- a/blockjob.c
> +++ b/blockjob.c
[...]
> @@ -247,7 +247,7 @@ void block_job_unref(BlockJob *job)
> block_job_detach_aio_context, job);
> blk_unref(job->blk);
> error_free(job->blocker);
> -g_free(job->id);
> +g_free(job->job.id);
Err. OK. I put my faith in patch 11.
Reviewed-by: Max Reitz
Although I do wonder about BlockJob.driver. It seems to stay even after
this series...
> assert(!timer_pending(&job->sleep_timer));
> g_free(job);
> }
[...]
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 459e3594e1..a97f60d104 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1369,6 +1369,8 @@ L: qemu-block@nongnu.org
(Cut off here:
M: Jeff Cody
Clever!)
> S: Supported
> F: blockjob.c
> F: include/block/blockjob.h
> +F: job.c
> +F: include/block/job.h
> F: block/backup.c
> F: block/commit.c
> F: block/stream.c
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [RFC 00/10] [TESTING NEEDED] python: futurize --stage1 (Python 3 compatibility)
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20180511222052.8734-1-ehabk...@redhat.com
Subject: [Qemu-devel] [RFC 00/10] [TESTING NEEDED] python: futurize --stage1
(Python 3 compatibility)
=== 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
git config --local diff.algorithm histogram
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
From https://github.com/patchew-project/qemu
t [tag update]patchew/20180510204148.11687-1-babu.mo...@amd.com ->
patchew/20180510204148.11687-1-babu.mo...@amd.com
* [new tag] patchew/20180511222052.8734-1-ehabk...@redhat.com ->
patchew/20180511222052.8734-1-ehabk...@redhat.com
Switched to a new branch 'test'
29e77f7e45 python: futurize -f lib2to3.fixes.fix_numliterals
06044c1048 python: futurize -f lib2to3.fixes.fix_except
aecdedee26 python: futurize -f lib2to3.fixes.fix_renames
2be7292f53 python: futurize -f lib2to3.fixes.fix_tuple_params
0b3e5f083c python: futurize -f lib2to3.fixes.fix_reduce
6d7a39f3c6 python: futurize -f lib2to3.fixes.fix_standarderror
0c8beeea09 python: futurize -f lib2to3.fixes.fix_has_key
4f0075d69d python: futurize -f libfuturize.fixes.fix_next_call
66698bae73 python: futurize -f libfuturize.fixes.fix_absolute_import
b48c4f449d python: futurize -f libfuturize.fixes.fix_print_with_import
=== OUTPUT BEGIN ===
Checking PATCH 1/10: python: futurize -f
libfuturize.fixes.fix_print_with_import...
ERROR: line over 90 characters
#40: FILE: scripts/analyse-9p-simpletrace.py:86:
+print("RERROR (tag =", tag, ", id =", symbol_9p[id], ", err =
\"", os.strerror(err), "\")")
ERROR: line over 90 characters
#44: FILE: scripts/analyse-9p-simpletrace.py:89:
+print("TVERSION (tag =", tag, ", msize =", msize, ", version
=", version, ")")
ERROR: line over 90 characters
#48: FILE: scripts/analyse-9p-simpletrace.py:92:
+print("RVERSION (tag =", tag, ", msize =", msize, ", version
=", version, ")")
ERROR: line over 90 characters
#52: FILE: scripts/analyse-9p-simpletrace.py:95:
+print("TATTACH (tag =", tag, ", fid =", fid, ", afid =", afid,
", uname =", uname, ", aname =", aname, ")")
ERROR: line over 90 characters
#56: FILE: scripts/analyse-9p-simpletrace.py:98:
+print("RATTACH (tag =", tag, ", qid={type =", type, ", version
=", version, ", path =", path, "})")
ERROR: line over 90 characters
#64: FILE: scripts/analyse-9p-simpletrace.py:104:
+print("RSTAT (tag =", tag, ", mode =", mode, ", atime =",
atime, ", mtime =", mtime, ", length =", length, ")")
ERROR: line over 90 characters
#68: FILE: scripts/analyse-9p-simpletrace.py:107:
+print("TGETATTR (tag =", tag, ", fid =", fid, ", request_mask
=", hex(request_mask), ")")
ERROR: line over 90 characters
#72: FILE: scripts/analyse-9p-simpletrace.py:110:
+print("RGETATTR (tag =", tag, ", result_mask =",
hex(result_mask), ", mode =", oct(mode), ", uid =", uid, ", gid =", gid, ")")
ERROR: line over 90 characters
#76: FILE: scripts/analyse-9p-simpletrace.py:113:
+print("TWALK (tag =", tag, ", fid =", fid, ", newfid =",
newfid, ", nwnames =", nwnames, ")")
ERROR: line over 90 characters
#80: FILE: scripts/analyse-9p-simpletrace.py:116:
+print("RWALK (tag =", tag, ", nwnames =", nwnames, ", qids =",
hex(qids), ")")
WARNING: line over 80 characters
#84: FILE: scripts/analyse-9p-simpletrace.py:119:
+print("TOPEN (tag =", tag, ", fid =", fid, ", mode =",
oct(mode), ")")
ERROR: line over 90 characters
#88: FILE: scripts/analyse-9p-simpletrace.py:122:
+print("ROPEN (tag =", tag, ", qid={type =", type, ", version
=", version, ", path =", path, "}, iounit =", iounit, ")")
ERROR: line over 90 characters
#92: FILE: scripts/analyse-9p-simpletrace.py:125:
+print("TLCREATE (tag =", tag, ", dfid =", dfid, ", flags =",
oct(flags), ", mode =", oct(mode), ", gid =", gid, ")")
ERROR: line over 90 characters
#96: FILE: scripts/analyse-9p-simpletrace.py:128:
+print("RLCREATE (tag =", tag, ", qid={type =", type, ",
version =", version, ", path =", path, "}, iounit =", iounit, ")")
WARNING: line over 80 characters
#100: FILE: scripts/analyse-9p-simpletrace.py:131:
+print("TFSYNC (tag =", tag, ", fid =", fid, ", datasync =",
datasync, ")")
ERROR: line over 90 characters
#108: FILE: scripts/analyse-9p-simpletrace.py:137:
+prin
Re: [Qemu-block] [PATCH 06/42] blockjob: Add block_job_driver()
On 2018-05-09 18:26, Kevin Wolf wrote: > The backup block job directly accesses the driver field in BlockJob. Add > a wrapper for getting it. > > Signed-off-by: Kevin Wolf > Reviewed-by: Eric Blake > --- > include/block/blockjob.h | 7 +++ > block/backup.c | 8 +--- > blockjob.c | 5 + > 3 files changed, 17 insertions(+), 3 deletions(-) Reviewed-by: Max Reitz signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH 07/42] blockjob: Remove block_job_pause/resume_all()
On 2018-05-09 18:26, Kevin Wolf wrote: > Commit 81193349 removed the only use of block_job_pause/resume_all(), > which was in bdrv_drain_all(). The functions are now unused and can be > removed. I have a strange liking for all-digit commit hash prefixes. > Signed-off-by: Kevin Wolf > --- > include/block/blockjob_int.h | 14 -- > blockjob.c | 27 --- > 2 files changed, 41 deletions(-) Reviewed-by: Max Reitz signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH 01/42] blockjob: Fix assertion in block_job_finalize()
On 05/09/2018 12:25 PM, Kevin Wolf wrote:
> Every job gets a non-NULL job->txn on creation, but it doesn't
> necessarily keep it until it is decommissioned: Finalising a job removes
> it from its transaction. Therefore, calling 'blockdev-job-finalize' a
> second time on an already concluded job causes an assertion failure.
>
> Remove job->txn from the assertion in block_job_finalize() to fix this.
> block_job_do_finalize() still has the same assertion, but if a job is
> already removed from its transaction, block_job_apply_verb() will
> already error out before we run into that assertion.
>
> Signed-off-by: Kevin Wolf
Reviewed-by: John Snow
> ---
> blockjob.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/blockjob.c b/blockjob.c
> index 4de48166b2..b38ed7e265 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -702,7 +702,7 @@ void block_job_complete(BlockJob *job, Error **errp)
>
> void block_job_finalize(BlockJob *job, Error **errp)
> {
> -assert(job && job->id && job->txn);
> +assert(job && job->id);
> if (block_job_apply_verb(job, BLOCK_JOB_VERB_FINALIZE, errp)) {
> return;
> }
>
Oh, ack. Good catch.
[Qemu-block] [RFC 10/10] python: futurize -f lib2to3.fixes.fix_numliterals
Convert octal literals into the new syntax.
This is necessary for Python 3 compatibility.
Done using:
$ py=$( (g grep -l -E '^#!.*python';find -name '*.py' -printf '%P\n';) | \
sort -u | grep -v README.sh4)
$ futurize -w -f lib2to3.fixes.fix_numliterals $py
Signed-off-by: Eduardo Habkost
---
scripts/qmp/qom-fuse | 6 +++---
tests/qemu-iotests/118 | 24
2 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/scripts/qmp/qom-fuse b/scripts/qmp/qom-fuse
index b00cb0a0af..e524e798fc 100755
--- a/scripts/qmp/qom-fuse
+++ b/scripts/qmp/qom-fuse
@@ -90,7 +90,7 @@ class QOMFS(Fuse):
def getattr(self, path):
if self.is_link(path):
-value = posix.stat_result((0755 | stat.S_IFLNK,
+value = posix.stat_result((0o755 | stat.S_IFLNK,
self.get_ino(path),
0,
2,
@@ -101,7 +101,7 @@ class QOMFS(Fuse):
0,
0))
elif self.is_object(path):
-value = posix.stat_result((0755 | stat.S_IFDIR,
+value = posix.stat_result((0o755 | stat.S_IFDIR,
self.get_ino(path),
0,
2,
@@ -112,7 +112,7 @@ class QOMFS(Fuse):
0,
0))
elif self.is_property(path):
-value = posix.stat_result((0644 | stat.S_IFREG,
+value = posix.stat_result((0o644 | stat.S_IFREG,
self.get_ino(path),
0,
1,
diff --git a/tests/qemu-iotests/118 b/tests/qemu-iotests/118
index a0469b570e..ff3b2ae3e7 100755
--- a/tests/qemu-iotests/118
+++ b/tests/qemu-iotests/118
@@ -390,14 +390,14 @@ class TestChangeReadOnly(ChangeBaseClass):
def tearDown(self):
self.vm.shutdown()
-os.chmod(old_img, 0666)
-os.chmod(new_img, 0666)
+os.chmod(old_img, 0o666)
+os.chmod(new_img, 0o666)
os.remove(old_img)
os.remove(new_img)
def test_ro_ro_retain(self):
-os.chmod(old_img, 0444)
-os.chmod(new_img, 0444)
+os.chmod(old_img, 0o444)
+os.chmod(new_img, 0o444)
self.vm.add_drive(old_img, 'media=disk,read-only=on', 'none')
self.vm.add_device('floppy,drive=drive0,id=%s' % self.device_name)
self.vm.launch()
@@ -417,7 +417,7 @@ class TestChangeReadOnly(ChangeBaseClass):
self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
def test_ro_rw_retain(self):
-os.chmod(old_img, 0444)
+os.chmod(old_img, 0o444)
self.vm.add_drive(old_img, 'media=disk,read-only=on', 'none')
self.vm.add_device('floppy,drive=drive0,id=%s' % self.device_name)
self.vm.launch()
@@ -437,7 +437,7 @@ class TestChangeReadOnly(ChangeBaseClass):
self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
def test_rw_ro_retain(self):
-os.chmod(new_img, 0444)
+os.chmod(new_img, 0o444)
self.vm.add_drive(old_img, 'media=disk', 'none')
self.vm.add_device('floppy,drive=drive0,id=%s' % self.device_name)
self.vm.launch()
@@ -459,7 +459,7 @@ class TestChangeReadOnly(ChangeBaseClass):
self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
def test_ro_rw(self):
-os.chmod(old_img, 0444)
+os.chmod(old_img, 0o444)
self.vm.add_drive(old_img, 'media=disk,read-only=on', 'none')
self.vm.add_device('floppy,drive=drive0,id=%s' % self.device_name)
self.vm.launch()
@@ -480,7 +480,7 @@ class TestChangeReadOnly(ChangeBaseClass):
self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
def test_rw_ro(self):
-os.chmod(new_img, 0444)
+os.chmod(new_img, 0o444)
self.vm.add_drive(old_img, 'media=disk', 'none')
self.vm.add_device('floppy,drive=drive0,id=%s' % self.device_name)
self.vm.launch()
@@ -521,7 +521,7 @@ class TestChangeReadOnly(ChangeBaseClass):
self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
def test_make_ro_rw(self):
-os.chmod(new_img, 0444)
+os.chmod(new_img, 0o444)
self.vm.add_drive(old_img, 'media=disk', 'none')
self.vm.add_device('floppy,drive=drive0,id=%s' % self.device_name)
self.vm.launch()
@@ -542,7 +542,7 @@ class TestChangeReadOnly(ChangeBaseClass):
self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
def test_make_rw_ro_by_retain(self):
-os.chmod(old_img, 0444)
+os.chmod(old_img, 0o444)
self.vm.add_drive(old_img, 'media=disk,read-only=on', 'none')
[Qemu-block] [RFC 06/10] python: futurize -f lib2to3.fixes.fix_reduce
Handle the move of reduce() to functools.reduce(). This is necessary for Python 3 compatibility. Done using: $ py=$( (g grep -l -E '^#!.*python';find -name '*.py' -printf '%P\n';) | \ sort -u | grep -v README.sh4) $ futurize -w -f lib2to3.fixes.fix_reduce $py Signed-off-by: Eduardo Habkost --- tests/image-fuzzer/qcow2/fuzz.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/image-fuzzer/qcow2/fuzz.py b/tests/image-fuzzer/qcow2/fuzz.py index 20eba6bc1b..abc4f0635d 100644 --- a/tests/image-fuzzer/qcow2/fuzz.py +++ b/tests/image-fuzzer/qcow2/fuzz.py @@ -17,6 +17,7 @@ # import random +from functools import reduce UINT8 = 0xff UINT16 = 0x -- 2.14.3
[Qemu-block] [RFC 05/10] python: futurize -f lib2to3.fixes.fix_standarderror
Rename StandardError to Exception.
This is necessary for Python 3 compatibility.
Done using:
$ py=$( (g grep -l -E '^#!.*python';find -name '*.py' -printf '%P\n';) | \
sort -u | grep -v README.sh4)
$ futurize -w -f lib2to3.fixes.fix_standarderror $py
Signed-off-by: Eduardo Habkost
---
scripts/qmp/qemu-ga-client | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/scripts/qmp/qemu-ga-client b/scripts/qmp/qemu-ga-client
index 6045fcd3f2..976e69e05f 100755
--- a/scripts/qmp/qemu-ga-client
+++ b/scripts/qmp/qemu-ga-client
@@ -137,7 +137,7 @@ class QemuGuestAgentClient:
def fsfreeze(self, cmd):
if cmd not in ['status', 'freeze', 'thaw']:
-raise StandardError('Invalid command: ' + cmd)
+raise Exception('Invalid command: ' + cmd)
return getattr(self.qga, 'fsfreeze' + '_' + cmd)()
@@ -146,7 +146,7 @@ class QemuGuestAgentClient:
def suspend(self, mode):
if mode not in ['disk', 'ram', 'hybrid']:
-raise StandardError('Invalid mode: ' + mode)
+raise Exception('Invalid mode: ' + mode)
try:
getattr(self.qga, 'suspend' + '_' + mode)()
@@ -157,7 +157,7 @@ class QemuGuestAgentClient:
def shutdown(self, mode='powerdown'):
if mode not in ['powerdown', 'halt', 'reboot']:
-raise StandardError('Invalid mode: ' + mode)
+raise Exception('Invalid mode: ' + mode)
try:
self.qga.shutdown(mode=mode)
--
2.14.3
[Qemu-block] [RFC 09/10] python: futurize -f lib2to3.fixes.fix_except
Convert "except X, T" to "except X as T".
This is necessary for Python 3 compatibility.
Done using:
$ py=$( (g grep -l -E '^#!.*python';find -name '*.py' -printf '%P\n';) | \
sort -u | grep -v README.sh4)
$ futurize -w -f lib2to3.fixes.fix_except $py
Signed-off-by: Eduardo Habkost
---
scripts/simpletrace.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/simpletrace.py b/scripts/simpletrace.py
index 2658b5cc7c..d4a50a1e2b 100755
--- a/scripts/simpletrace.py
+++ b/scripts/simpletrace.py
@@ -45,7 +45,7 @@ def get_record(edict, idtoname, rechdr, fobj):
rec = (name, rechdr[1], rechdr[3])
try:
event = edict[name]
-except KeyError, e:
+except KeyError as e:
import sys
sys.stderr.write('%s event is logged but is not declared ' \
'in the trace events file, try using ' \
--
2.14.3
Re: [Qemu-block] [PATCH 05/42] blockjob: Introduce block_job_ratelimit_get_delay()
On 2018-05-09 18:26, Kevin Wolf wrote: > This gets us rid of more direct accesses to BlockJob fields from the > job drivers. > > Signed-off-by: Kevin Wolf > Reviewed-by: Eric Blake > --- > include/block/blockjob_int.h | 8 > block/backup.c | 18 +++--- > block/commit.c | 4 ++-- > block/mirror.c | 5 + > block/stream.c | 4 ++-- > blockjob.c | 9 + > 6 files changed, 29 insertions(+), 19 deletions(-) Reviewed-by: Max Reitz signature.asc Description: OpenPGP digital signature
[Qemu-block] [RFC 07/10] python: futurize -f lib2to3.fixes.fix_tuple_params
Remove implicit tuple parameter unpacking.
This is necessary for Python 3 compatibility.
Done using:
$ py=$( (g grep -l -E '^#!.*python';find -name '*.py' -printf '%P\n';) | \
sort -u | grep -v README.sh4)
$ futurize -w -f lib2to3.fixes.fix_tuple_params $py
Signed-off-by: Eduardo Habkost
---
scripts/analyse-locks-simpletrace.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/analyse-locks-simpletrace.py
b/scripts/analyse-locks-simpletrace.py
index 352bc9c22d..30090bdfff 100755
--- a/scripts/analyse-locks-simpletrace.py
+++ b/scripts/analyse-locks-simpletrace.py
@@ -78,7 +78,7 @@ if __name__ == '__main__':
# Now dump the individual lock stats
for key, val in sorted(analyser.mutex_records.iteritems(),
- key=lambda (k,v): v["locks"]):
+ key=lambda k_v: k_v[1]["locks"]):
print ("Lock: %#x locks: %d, locked: %d, unlocked: %d" %
(key, val["locks"], val["locked"], val["unlocked"]))
--
2.14.3
[Qemu-block] [RFC 08/10] python: futurize -f lib2to3.fixes.fix_renames
Change sys.maxint to sys.maxsize. This is necessary for Python 3 compatibility. Done using: $ py=$( (g grep -l -E '^#!.*python';find -name '*.py' -printf '%P\n';) | \ sort -u | grep -v README.sh4) $ futurize -w -f lib2to3.fixes.fix_renames $py Signed-off-by: Eduardo Habkost --- tests/image-fuzzer/runner.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/image-fuzzer/runner.py b/tests/image-fuzzer/runner.py index 45e8fca63f..95d84f38f3 100755 --- a/tests/image-fuzzer/runner.py +++ b/tests/image-fuzzer/runner.py @@ -128,7 +128,7 @@ class TestEnv(object): if seed is not None: self.seed = seed else: -self.seed = str(random.randint(0, sys.maxint)) +self.seed = str(random.randint(0, sys.maxsize)) random.seed(self.seed) self.init_path = os.getcwd() -- 2.14.3
[Qemu-block] [RFC 01/10] python: futurize -f libfuturize.fixes.fix_print_with_import
Change all Python code to use print as a function.
This is necessary for Python 3 compatibility.
Done using:
$ py=$( (g grep -l -E '^#!.*python';find -name '*.py' -printf '%P\n';) | \
sort -u | grep -v README.sh4)
$ futurize -w -f libfuturize.fixes.fix_print_with_import $py
Signed-off-by: Eduardo Habkost
---
scripts/analyse-9p-simpletrace.py| 89
scripts/analyse-locks-simpletrace.py | 1 +
scripts/analyze-migration.py | 11 ++--
scripts/dump-guest-memory.py | 1 +
scripts/replay-dump.py | 21
scripts/signrom.py | 1 +
scripts/simpletrace.py | 3 +-
scripts/vmstate-static-checker.py| 85 +++---
scripts/device-crash-test| 3 +-
scripts/kvm/kvm_flightrecorder | 21
scripts/kvm/vmxcap | 1 +
scripts/qmp/qemu-ga-client | 1 +
scripts/qmp/qmp | 17 +++---
scripts/qmp/qmp-shell| 35 +++--
scripts/qmp/qom-get | 7 +--
scripts/qmp/qom-list | 11 ++--
scripts/qmp/qom-set | 5 +-
scripts/qmp/qom-tree | 11 ++--
tests/docker/docker.py | 11 ++--
tests/docker/travis.py | 15 +++---
tests/guest-debug/test-gdbstub.py| 1 +
tests/image-fuzzer/runner.py | 38 ++
tests/migration/guestperf/engine.py | 29 ++-
tests/migration/guestperf/plot.py| 17 +++---
tests/migration/guestperf/shell.py | 19 +++
tests/qemu-iotests/149 | 3 +-
tests/qemu-iotests/165 | 3 +-
tests/qemu-iotests/iotests.py| 5 +-
tests/qemu-iotests/nbd-fault-injector.py | 7 +--
tests/qemu-iotests/qcow2.py | 39 +++---
tests/qemu-iotests/qed.py| 17 +++---
tests/vm/basevm.py | 3 +-
32 files changed, 278 insertions(+), 253 deletions(-)
diff --git a/scripts/analyse-9p-simpletrace.py
b/scripts/analyse-9p-simpletrace.py
index 3c3dee4337..710e01adba 100755
--- a/scripts/analyse-9p-simpletrace.py
+++ b/scripts/analyse-9p-simpletrace.py
@@ -3,6 +3,7 @@
# Usage: ./analyse-9p-simpletrace
#
# Author: Harsh Prateek Bora
+from __future__ import print_function
import os
import simpletrace
@@ -79,135 +80,135 @@ symbol_9p = {
class VirtFSRequestTracker(simpletrace.Analyzer):
def begin(self):
-print "Pretty printing 9p simpletrace log ..."
+print("Pretty printing 9p simpletrace log ...")
def v9fs_rerror(self, tag, id, err):
-print "RERROR (tag =", tag, ", id =", symbol_9p[id], ", err =
\"", os.strerror(err), "\")"
+print("RERROR (tag =", tag, ", id =", symbol_9p[id], ", err =
\"", os.strerror(err), "\")")
def v9fs_version(self, tag, id, msize, version):
-print "TVERSION (tag =", tag, ", msize =", msize, ", version
=", version, ")"
+print("TVERSION (tag =", tag, ", msize =", msize, ", version
=", version, ")")
def v9fs_version_return(self, tag, id, msize, version):
-print "RVERSION (tag =", tag, ", msize =", msize, ", version
=", version, ")"
+print("RVERSION (tag =", tag, ", msize =", msize, ", version
=", version, ")")
def v9fs_attach(self, tag, id, fid, afid, uname, aname):
-print "TATTACH (tag =", tag, ", fid =", fid, ", afid =", afid,
", uname =", uname, ", aname =", aname, ")"
+print("TATTACH (tag =", tag, ", fid =", fid, ", afid =", afid,
", uname =", uname, ", aname =", aname, ")")
def v9fs_attach_return(self, tag, id, type, version, path):
-print "RATTACH (tag =", tag, ", qid={type =", type, ", version
=", version, ", path =", path, "})"
+print("RATTACH (tag =", tag, ", qid={type =", type, ", version
=", version, ", path =", path, "})")
def v9fs_stat(self, tag, id, fid):
-print "TSTAT (tag =", tag, ", fid =", fid, ")"
+print("TSTAT (tag =", tag, ", fid =", fid, ")")
def v9fs_stat_return(self, tag, id, mode, atime, mtime, length):
-print "RSTAT (tag =", tag, ", mode =", mode, ", atime =",
atime, ", mtime =", mtime, ", length =", length, ")"
+print("RSTAT (tag =", tag, ", mode =", mode, ", atime =",
atime, ", mtime =", mtime, ", length =", length, ")")
def v9fs_getattr(self, tag, id, fid, request_mask):
-print "TGETATTR (tag =", tag, ", fid =", fid, ", request_mask
=", hex(request_mask), ")"
+print("TGETATTR (tag =", tag, ", fid =", fid, ", request_mask
=", hex(request_mask), ")")
def v9fs_getattr_return(self, tag, id, result_mask, mode, uid, gid):
-
[Qemu-block] [RFC 04/10] python: futurize -f lib2to3.fixes.fix_has_key
Change "dict.has_key(key)" to "key in dict"
This is necessary for Python 3 compatibility.
Done using:
$ py=$( (g grep -l -E '^#!.*python';find -name '*.py' -printf '%P\n';) | \
sort -u | grep -v README.sh4)
$ futurize -w -f lib2to3.fixes.fix_has_key $py
Signed-off-by: Eduardo Habkost
---
scripts/qmp/qmp| 4 ++--
scripts/qmp/qmp-shell | 2 +-
scripts/qmp/qom-fuse | 2 +-
scripts/qmp/qom-get| 2 +-
scripts/qmp/qom-list | 2 +-
scripts/qmp/qom-set| 2 +-
scripts/qmp/qom-tree | 2 +-
tests/qemu-iotests/093 | 2 +-
tests/qemu-iotests/096 | 4 ++--
tests/qemu-iotests/136 | 2 +-
10 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/scripts/qmp/qmp b/scripts/qmp/qmp
index 4d2be4e98a..33a0d6b73a 100755
--- a/scripts/qmp/qmp
+++ b/scripts/qmp/qmp
@@ -36,7 +36,7 @@ def main(args):
path = None
# Use QMP_PATH if it's set
-if os.environ.has_key('QMP_PATH'):
+if 'QMP_PATH' in os.environ:
path = os.environ['QMP_PATH']
while len(args):
@@ -80,7 +80,7 @@ def main(args):
def do_command(srv, cmd, **kwds):
rsp = srv.cmd(cmd, kwds)
-if rsp.has_key('error'):
+if 'error' in rsp:
raise Exception(rsp['error']['desc'])
return rsp['return']
diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
index 38c99d8f72..26418dab95 100755
--- a/scripts/qmp/qmp-shell
+++ b/scripts/qmp/qmp-shell
@@ -134,7 +134,7 @@ class QMPShell(qmp.QEMUMonitorProtocol):
def _fill_completion(self):
cmds = self.cmd('query-commands')
-if cmds.has_key('error'):
+if 'error' in cmds:
return
for cmd in cmds['return']:
self._completer.append(cmd['name'])
diff --git a/scripts/qmp/qom-fuse b/scripts/qmp/qom-fuse
index b75aa72767..b00cb0a0af 100755
--- a/scripts/qmp/qom-fuse
+++ b/scripts/qmp/qom-fuse
@@ -29,7 +29,7 @@ class QOMFS(Fuse):
self.ino_count = 1
def get_ino(self, path):
-if self.ino_map.has_key(path):
+if path in self.ino_map:
return self.ino_map[path]
self.ino_map[path] = self.ino_count
self.ino_count += 1
diff --git a/scripts/qmp/qom-get b/scripts/qmp/qom-get
index 6313f27e8e..a3f5d7660e 100755
--- a/scripts/qmp/qom-get
+++ b/scripts/qmp/qom-get
@@ -45,7 +45,7 @@ if len(args) > 0:
args = args[2:]
if not socket_path:
-if os.environ.has_key('QMP_SOCKET'):
+if 'QMP_SOCKET' in os.environ:
socket_path = os.environ['QMP_SOCKET']
else:
usage_error("no QMP socket path or address given");
diff --git a/scripts/qmp/qom-list b/scripts/qmp/qom-list
index 80b0a3d1be..2ba25e1792 100755
--- a/scripts/qmp/qom-list
+++ b/scripts/qmp/qom-list
@@ -45,7 +45,7 @@ if len(args) > 0:
args = args[2:]
if not socket_path:
-if os.environ.has_key('QMP_SOCKET'):
+if 'QMP_SOCKET' in os.environ:
socket_path = os.environ['QMP_SOCKET']
else:
usage_error("no QMP socket path or address given");
diff --git a/scripts/qmp/qom-set b/scripts/qmp/qom-set
index cbffb65880..0352668812 100755
--- a/scripts/qmp/qom-set
+++ b/scripts/qmp/qom-set
@@ -46,7 +46,7 @@ if len(args) > 0:
args = args[2:]
if not socket_path:
-if os.environ.has_key('QMP_SOCKET'):
+if 'QMP_SOCKET' in os.environ:
socket_path = os.environ['QMP_SOCKET']
else:
usage_error("no QMP socket path or address given");
diff --git a/scripts/qmp/qom-tree b/scripts/qmp/qom-tree
index ad4be233e6..32e708a13e 100755
--- a/scripts/qmp/qom-tree
+++ b/scripts/qmp/qom-tree
@@ -47,7 +47,7 @@ if len(args) > 0:
args = args[2:]
if not socket_path:
-if os.environ.has_key('QMP_SOCKET'):
+if 'QMP_SOCKET' in os.environ:
socket_path = os.environ['QMP_SOCKET']
else:
usage_error("no QMP socket path or address given");
diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
index c3404a3171..68e344f8c1 100755
--- a/tests/qemu-iotests/093
+++ b/tests/qemu-iotests/093
@@ -237,7 +237,7 @@ class ThrottleTestGroupNames(iotests.QMPTestCase):
if name:
self.assertEqual(info["group"], name)
else:
-self.assertFalse(info.has_key('group'))
+self.assertFalse('group' in info)
return
raise Exception("No group information found for '%s'" % device)
diff --git a/tests/qemu-iotests/096 b/tests/qemu-iotests/096
index aeeb3753cf..a69439602d 100755
--- a/tests/qemu-iotests/096
+++ b/tests/qemu-iotests/096
@@ -53,9 +53,9 @@ class TestLiveSnapshot(iotests.QMPTestCase):
self.assertEqual(r['iops'], self.iops)
self.assertEqual(r['iops_size'], self.iops_size)
else:
-self.assertFalse(r.has_key('group'))
+self.assertFalse('group' in r)
self.assertEqual(r['iops'], 0)
-self.assertFalse(r.has_key('iops_size'))
+
[Qemu-block] [RFC 02/10] python: futurize -f libfuturize.fixes.fix_absolute_import
Make implicit relative imports explicit and add "from __future__ import absolute_import" at the top of each relevant module. This is necessary for Python 3 compatibility. Done using: $ py=$( (g grep -l -E '^#!.*python';find -name '*.py' -printf '%P\n';) | \ sort -u | grep -v README.sh4) $ futurize -w -f libfuturize.fixes.fix_absolute_import $py Signed-off-by: Eduardo Habkost --- scripts/qmp/qemu-ga-client | 3 ++- scripts/qmp/qmp | 3 ++- scripts/qmp/qmp-shell| 3 ++- scripts/qmp/qom-fuse | 3 ++- scripts/qmp/qom-get | 3 ++- scripts/qmp/qom-list | 3 ++- scripts/qmp/qom-set | 3 ++- scripts/qmp/qom-tree | 3 ++- tests/image-fuzzer/qcow2/__init__.py | 3 ++- tests/image-fuzzer/qcow2/layout.py | 3 ++- 10 files changed, 20 insertions(+), 10 deletions(-) diff --git a/scripts/qmp/qemu-ga-client b/scripts/qmp/qemu-ga-client index 8510814683..6045fcd3f2 100755 --- a/scripts/qmp/qemu-ga-client +++ b/scripts/qmp/qemu-ga-client @@ -37,10 +37,11 @@ # from __future__ import print_function +from __future__ import absolute_import import base64 import random -import qmp +from . import qmp class QemuGuestAgent(qmp.QEMUMonitorProtocol): diff --git a/scripts/qmp/qmp b/scripts/qmp/qmp index 16d3bdb6fe..4d2be4e98a 100755 --- a/scripts/qmp/qmp +++ b/scripts/qmp/qmp @@ -11,8 +11,9 @@ # See the COPYING file in the top-level directory. from __future__ import print_function +from __future__ import absolute_import import sys, os -from qmp import QEMUMonitorProtocol +from .qmp import QEMUMonitorProtocol def print_response(rsp, prefix=[]): if type(rsp) == list: diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell index b1cc7e2271..38c99d8f72 100755 --- a/scripts/qmp/qmp-shell +++ b/scripts/qmp/qmp-shell @@ -66,7 +66,8 @@ # sent to QEMU, which is useful for debugging and documentation generation. from __future__ import print_function -import qmp +from __future__ import absolute_import +from . import qmp import json import ast import readline diff --git a/scripts/qmp/qom-fuse b/scripts/qmp/qom-fuse index 5c6754aa63..b75aa72767 100755 --- a/scripts/qmp/qom-fuse +++ b/scripts/qmp/qom-fuse @@ -11,11 +11,12 @@ # the COPYING file in the top-level directory. ## +from __future__ import absolute_import import fuse, stat from fuse import Fuse import os, posix from errno import * -from qmp import QEMUMonitorProtocol +from .qmp import QEMUMonitorProtocol fuse.fuse_python_api = (0, 2) diff --git a/scripts/qmp/qom-get b/scripts/qmp/qom-get index 291c8bfbc2..6313f27e8e 100755 --- a/scripts/qmp/qom-get +++ b/scripts/qmp/qom-get @@ -12,9 +12,10 @@ ## from __future__ import print_function +from __future__ import absolute_import import sys import os -from qmp import QEMUMonitorProtocol +from .qmp import QEMUMonitorProtocol cmd, args = sys.argv[0], sys.argv[1:] socket_path = None diff --git a/scripts/qmp/qom-list b/scripts/qmp/qom-list index cd907bb81f..80b0a3d1be 100755 --- a/scripts/qmp/qom-list +++ b/scripts/qmp/qom-list @@ -12,9 +12,10 @@ ## from __future__ import print_function +from __future__ import absolute_import import sys import os -from qmp import QEMUMonitorProtocol +from .qmp import QEMUMonitorProtocol cmd, args = sys.argv[0], sys.argv[1:] socket_path = None diff --git a/scripts/qmp/qom-set b/scripts/qmp/qom-set index fbe4b3e471..cbffb65880 100755 --- a/scripts/qmp/qom-set +++ b/scripts/qmp/qom-set @@ -12,9 +12,10 @@ ## from __future__ import print_function +from __future__ import absolute_import import sys import os -from qmp import QEMUMonitorProtocol +from .qmp import QEMUMonitorProtocol cmd, args = sys.argv[0], sys.argv[1:] socket_path = None diff --git a/scripts/qmp/qom-tree b/scripts/qmp/qom-tree index 0ffd1ff1de..ad4be233e6 100755 --- a/scripts/qmp/qom-tree +++ b/scripts/qmp/qom-tree @@ -14,9 +14,10 @@ ## from __future__ import print_function +from __future__ import absolute_import import sys import os -from qmp import QEMUMonitorProtocol +from .qmp import QEMUMonitorProtocol cmd, args = sys.argv[0], sys.argv[1:] socket_path = None diff --git a/tests/image-fuzzer/qcow2/__init__.py b/tests/image-fuzzer/qcow2/__init__.py index e2ebe19311..09ef59821b 100644 --- a/tests/image-fuzzer/qcow2/__init__.py +++ b/tests/image-fuzzer/qcow2/__init__.py @@ -1 +1,2 @@ -from layout import create_image +from __future__ import absolute_import +from .layout import create_image diff --git a/tests/image-fuzzer/qcow2/layout.py b/tests/image-fuzzer/qcow2/layout.py index 63e801f4e8..df2131a855 100644 --- a/tests/image-fuzzer/qcow2/layout.py +++ b/tests/image-fuzzer/qcow2/layout.py @@ -1,3 +1,4 @@ +from __future__ import absolute_import # Generator of fuzzed qcow2 images # # Copyright (C) 2014 Maria Kustova @@ -18,7 +19,7 @@ import random import struct -import fuzz +from . import fuzz from math import ceil from
[Qemu-block] [RFC 03/10] python: futurize -f libfuturize.fixes.fix_next_call
Change obj.next() calls to next(obj).
This is necessary for Python 3 compatibility.
Done using:
$ py=$( (g grep -l -E '^#!.*python';find -name '*.py' -printf '%P\n';) | \
sort -u | grep -v README.sh4)
$ futurize -w -f libfuturize.fixes.fix_next_call $py
Signed-off-by: Eduardo Habkost
---
scripts/ordereddict.py| 4 ++--
scripts/vmstate-static-checker.py | 4 ++--
tests/image-fuzzer/runner.py | 2 +-
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/scripts/ordereddict.py b/scripts/ordereddict.py
index 2d1d81370b..68ed340b33 100644
--- a/scripts/ordereddict.py
+++ b/scripts/ordereddict.py
@@ -71,9 +71,9 @@ class OrderedDict(dict, DictMixin):
if not self:
raise KeyError('dictionary is empty')
if last:
-key = reversed(self).next()
+key = next(reversed(self))
else:
-key = iter(self).next()
+key = next(iter(self))
value = self.pop(key)
return key, value
diff --git a/scripts/vmstate-static-checker.py
b/scripts/vmstate-static-checker.py
index 60d1bf4cda..d3467288dc 100755
--- a/scripts/vmstate-static-checker.py
+++ b/scripts/vmstate-static-checker.py
@@ -158,7 +158,7 @@ def check_fields(src_fields, dest_fields, desc, sec):
while True:
if advance_src:
try:
-s_item = s_iter.next()
+s_item = next(s_iter)
except StopIteration:
if s_iter_list == []:
break
@@ -173,7 +173,7 @@ def check_fields(src_fields, dest_fields, desc, sec):
if advance_dest:
try:
-d_item = d_iter.next()
+d_item = next(d_iter)
except StopIteration:
if d_iter_list == []:
# We were not in a substruct
diff --git a/tests/image-fuzzer/runner.py b/tests/image-fuzzer/runner.py
index 8de656933e..45e8fca63f 100755
--- a/tests/image-fuzzer/runner.py
+++ b/tests/image-fuzzer/runner.py
@@ -422,7 +422,7 @@ if __name__ == '__main__':
test_id = count(1)
while should_continue(duration, start_time):
try:
-run_test(str(test_id.next()), seed, work_dir, run_log, cleanup,
+run_test(str(next(test_id)), seed, work_dir, run_log, cleanup,
log_all, command, config)
except (KeyboardInterrupt, SystemExit):
sys.exit(1)
--
2.14.3
[Qemu-block] [RFC 00/10] [TESTING NEEDED] python: futurize --stage1 (Python 3 compatibility)
TESTING NEEDED: Due to the amount of changes, I didn't test all scripts touched by this series. If you are responsible for any of the touched files, I would appreciate help on testing the series. >From the futurize[1] documentation: > This applies fixes that modernize Python 2 code without > changing the effect of the code. With luck, this will not > introduce any bugs into the code, or will at least be trivial > to fix. The changes are those that bring the Python code > up-to-date without breaking Py2 compatibility. The resulting > code will be modern Python 2.6-compatible code plus __future__ > imports from the following set: > > from __future__ import absolute_import > from __future__ import division > from __future__ import print_function > [...] > The goal for this stage is to create most of the diff for the > entire porting process, but without introducing any bugs. It > should be uncontroversial and safe to apply to every Python 2 > package. The subsequent patches introducing Python 3 > compatibility should then be shorter and easier to review. This series run all the fixers from futurize --stage1 on all Python code in the tree. To make review and testing easier, I have run the fixers separately instead of doing all changes in a single patch. [1] http://python-future.org/automatic_conversion.html Eduardo Habkost (10): python: futurize -f libfuturize.fixes.fix_print_with_import python: futurize -f libfuturize.fixes.fix_absolute_import python: futurize -f libfuturize.fixes.fix_next_call python: futurize -f lib2to3.fixes.fix_has_key python: futurize -f lib2to3.fixes.fix_standarderror python: futurize -f lib2to3.fixes.fix_reduce python: futurize -f lib2to3.fixes.fix_tuple_params python: futurize -f lib2to3.fixes.fix_renames python: futurize -f lib2to3.fixes.fix_except python: futurize -f lib2to3.fixes.fix_numliterals scripts/analyse-9p-simpletrace.py| 89 scripts/analyse-locks-simpletrace.py | 3 +- scripts/analyze-migration.py | 11 ++-- scripts/dump-guest-memory.py | 1 + scripts/ordereddict.py | 4 +- scripts/replay-dump.py | 21 scripts/signrom.py | 1 + scripts/simpletrace.py | 5 +- scripts/vmstate-static-checker.py| 89 scripts/device-crash-test| 3 +- scripts/kvm/kvm_flightrecorder | 21 scripts/kvm/vmxcap | 1 + scripts/qmp/qemu-ga-client | 10 ++-- scripts/qmp/qmp | 24 + scripts/qmp/qmp-shell| 40 +++--- scripts/qmp/qom-fuse | 11 ++-- scripts/qmp/qom-get | 12 +++-- scripts/qmp/qom-list | 16 +++--- scripts/qmp/qom-set | 10 ++-- scripts/qmp/qom-tree | 16 +++--- tests/docker/docker.py | 11 ++-- tests/docker/travis.py | 15 +++--- tests/guest-debug/test-gdbstub.py| 1 + tests/image-fuzzer/qcow2/__init__.py | 3 +- tests/image-fuzzer/qcow2/fuzz.py | 1 + tests/image-fuzzer/qcow2/layout.py | 3 +- tests/image-fuzzer/runner.py | 42 +++ tests/migration/guestperf/engine.py | 29 ++- tests/migration/guestperf/plot.py| 17 +++--- tests/migration/guestperf/shell.py | 19 +++ tests/qemu-iotests/093 | 2 +- tests/qemu-iotests/096 | 4 +- tests/qemu-iotests/118 | 24 - tests/qemu-iotests/136 | 2 +- tests/qemu-iotests/149 | 3 +- tests/qemu-iotests/165 | 3 +- tests/qemu-iotests/iotests.py| 5 +- tests/qemu-iotests/nbd-fault-injector.py | 7 +-- tests/qemu-iotests/qcow2.py | 39 +++--- tests/qemu-iotests/qed.py| 17 +++--- tests/vm/basevm.py | 3 +- 41 files changed, 337 insertions(+), 301 deletions(-) -- 2.14.3
Re: [Qemu-block] [PATCH 04/42] blockjob: Implement block_job_set_speed() centrally
On 2018-05-09 18:25, Kevin Wolf wrote: > All block job drivers support .set_speed and all of them duplicate the > same code to implement it. Move that code to blockjob.c and remove the > now useless callback. > > Signed-off-by: Kevin Wolf > Reviewed-by: Eric Blake > --- > include/block/blockjob.h | 2 ++ > include/block/blockjob_int.h | 3 --- > block/backup.c | 13 - > block/commit.c | 14 -- > block/mirror.c | 14 -- > block/stream.c | 14 -- > blockjob.c | 12 > 7 files changed, 6 insertions(+), 66 deletions(-) > > diff --git a/include/block/blockjob.h b/include/block/blockjob.h > index 22bf418209..5aa8a6aaec 100644 > --- a/include/block/blockjob.h > +++ b/include/block/blockjob.h > @@ -29,6 +29,8 @@ > #include "block/block.h" > #include "qemu/ratelimit.h" > > +#define SLICE_TIME 1ULL /* ns */ > + > typedef struct BlockJobDriver BlockJobDriver; > typedef struct BlockJobTxn BlockJobTxn; SLICE_TIME can be anything. I don't like something that can be anything to be in a header file. I can see that you still need it in mirror, so it needs to be in a header; but maybe rename it to... THROTTLE_SLICE_TIME? At least JOB_SLICE_TIME? Apart from that: Reviewed-by: Max Reitz signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH 03/42] blockjob: Move RateLimit to BlockJob
On 2018-05-09 18:25, Kevin Wolf wrote: > Every block job has a RateLimit, and they all do the exact same thing > with it, so it should be common infrastructure. Move the struct field > for a start. > > Signed-off-by: Kevin Wolf > Reviewed-by: Eric Blake > --- > include/block/blockjob.h | 4 > block/backup.c | 5 ++--- > block/commit.c | 5 ++--- > block/mirror.c | 6 +++--- > block/stream.c | 5 ++--- > 5 files changed, 13 insertions(+), 12 deletions(-) Instead of finally getting rid of block job throttling, you make it central functionality? Bah, I say, bah! ;-) Reviewed-by: Max Reitz signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH 02/42] blockjob: Wrappers for progress counter access
On 2018-05-09 18:25, Kevin Wolf wrote: > Block job drivers are not expected to mess with the internals of the > BlockJob object, so provide wrapper functions for one of the cases where > they still do it: Updating the progress counter. > > Signed-off-by: Kevin Wolf > Reviewed-by: Eric Blake > --- > include/block/blockjob.h | 19 +++ > block/backup.c | 22 +- > block/commit.c | 16 > block/mirror.c | 11 +-- > block/stream.c | 14 -- > blockjob.c | 10 ++ > 6 files changed, 63 insertions(+), 29 deletions(-) > [...] > diff --git a/block/backup.c b/block/backup.c > index 453cd62c24..5d95805472 100644 > --- a/block/backup.c > +++ b/block/backup.c > [...] > @@ -420,8 +421,9 @@ static void > backup_incremental_init_copy_bitmap(BackupBlockJob *job) > bdrv_set_dirty_iter(dbi, next_cluster * job->cluster_size); > } > > -job->common.offset = job->common.len - > - hbitmap_count(job->copy_bitmap) * job->cluster_size; > +/* TODO block_job_progress_set_remaining() would make more sense */ Extremely true, especially considering that at least there was an assignment before. > +block_job_progress_update(&job->common, > +job->len - hbitmap_count(job->copy_bitmap) * job->cluster_size); Now, with an incremental update, you have to know that the progress was 0 before this call to make any sense of it. I could ask: Why don't you just resolve the TODO immediately with block_job_progress_set_remaining(&job->common, hbitmap_count(job->copy_bitmap) * job->cluster_size); ? I suppose one possible answer is that this series has 42 patches as it is, but I have to say that it took me more time to figure this hunk out than it would have taken me to acknowledge the above change. Considering that job->len and job->common.len are now separate after this patch, and that there is only a single other block_job_progress_update() call in this file, I can't see any side effects. > > bdrv_dirty_iter_free(dbi); > } [...] > diff --git a/block/mirror.c b/block/mirror.c > index 99da9c0858..77ee9b1791 100644 > --- a/block/mirror.c > +++ b/block/mirror.c [...] > @@ -792,11 +792,10 @@ static void coroutine_fn mirror_run(void *opaque) > block_job_pause_point(&s->common); > > cnt = bdrv_get_dirty_count(s->dirty_bitmap); > -/* s->common.offset contains the number of bytes already processed so > - * far, cnt is the number of dirty bytes remaining and > - * s->bytes_in_flight is the number of bytes currently being > - * processed; together those are the current total operation length > */ > -s->common.len = s->common.offset + s->bytes_in_flight + cnt; > +/* cnt is the number of dirty bytes remaining and s->bytes_in_flight > is > + * the number of bytes currently being processed; together those are > + * the current total operation length */ No, together, those are the current remaining operation length. With that fixed: Reviewed-by: Max Reitz > +block_job_progress_set_remaining(&s->common, s->bytes_in_flight + > cnt); > > /* Note that even when no rate limit is applied we need to yield > * periodically with no pending I/O so that bdrv_drain_all() returns. signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH 01/42] blockjob: Fix assertion in block_job_finalize()
On 2018-05-09 18:25, Kevin Wolf wrote: > Every job gets a non-NULL job->txn on creation, but it doesn't > necessarily keep it until it is decommissioned: Finalising a job removes > it from its transaction. Therefore, calling 'blockdev-job-finalize' a > second time on an already concluded job causes an assertion failure. > > Remove job->txn from the assertion in block_job_finalize() to fix this. > block_job_do_finalize() still has the same assertion, but if a job is > already removed from its transaction, block_job_apply_verb() will > already error out before we run into that assertion. > > Signed-off-by: Kevin Wolf > --- > blockjob.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Max Reitz signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH 07/13] qdict: Add qdict_stringify_for_keyval()
On 2018-05-11 20:39, Eric Blake wrote:
> On 05/09/2018 11:55 AM, Max Reitz wrote:
>> The purpose of this function is to prepare a QDict for consumption by
>> the keyval visitor, which only accepts strings and QNull.
>>
>> Signed-off-by: Max Reitz
>> ---
>> include/block/qdict.h | 2 ++
>> qobject/qdict.c | 57
>> +++
>> 2 files changed, 59 insertions(+)
>>
>
>> +/**
>> + * Convert all values in a QDict so it can be consumed by the keyval
>> + * input visitor. QNull values are left as-is, all other values are
>> + * converted to strings.
>> + *
>> + * @qdict must be flattened, i.e. it may not contain any nested QDicts
>> + * or QLists.
>
> Maybe s/flattened/flattened already/
>
> or would it be worth letting this function PERFORM the flattening step
> automatically?
Possibly, but I don't think it would be any more efficient, so I'd
rather leave it up to the caller to do it explicitly than to do it here
and maybe surprise someone.
(Indeed I personally prefer to be surprised by an abort() than by some
unintended data change.)
Max
>> + */
>> +void qdict_stringify_for_keyval(QDict *qdict)
>> +{
>> + const QDictEntry *e;
>> +
>> + for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
>> + QString *new_value = NULL;
>> +
>> + switch (qobject_type(e->value)) {
>> + case QTYPE_QNULL:
>> + /* leave as-is */
>> + break;
>> +
>> + case QTYPE_QNUM: {
>> + char *str = qnum_to_string(qobject_to(QNum, e->value));
>> + new_value = qstring_from_str(str);
>> + g_free(str);
>> + break;
>> + }
>> +
>> + case QTYPE_QSTRING:
>> + /* leave as-is */
>> + break;
>> +
>> + case QTYPE_QDICT:
>> + case QTYPE_QLIST:
>> + /* @qdict must be flattened */
>> + abort();
>
> Matches your documentation about requiring it to be already flattened.
>
>> +
>> + case QTYPE_QBOOL:
>> + if (qbool_get_bool(qobject_to(QBool, e->value))) {
>> + new_value = qstring_from_str("on");
>> + } else {
>> + new_value = qstring_from_str("off");
>> + }
>> + break;
>> +
>> + case QTYPE_NONE:
>> + case QTYPE__MAX:
>> + abort();
>> + }
>> +
>> + if (new_value) {
>> + QDictEntry *mut_e = (QDictEntry *)e;
>
> A bit of a shame that you have to cast away const. It took me a couple
> reads to figure out this meant 'mutable_e'. But in the end, the code
> looks correct.
>
>> + qobject_unref(mut_e->value);
>> + mut_e->value = QOBJECT(new_value);
>
> If we're okay requiring the caller to pre-flatten before calling this,
> instead of offering to do it here,
> Reviewed-by: Eric Blake
>
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH 10/13] tests: Add QDict clone-flatten test
On 2018-05-11 20:46, Eric Blake wrote:
> On 05/09/2018 11:55 AM, Max Reitz wrote:
>> This new test verifies that qdict_flatten() does not modify a shallow
>> clone of the given QDict.
>>
>> Signed-off-by: Max Reitz
>> ---
>> tests/check-qdict.c | 33 +
>> 1 file changed, 33 insertions(+)
>>
>
> I'm not sure I even want to know how long it took you to debug the crash
> that you obviously hit before adding the fix in 9/13 plus this test ;)
Thank you very much, I made myself forget about that trauma already.
In short, I wondered why the whole thing worked for null-co directly:
driver=null-co,size=512
=> {"driver": "null-co", "size": 512}
But not for null-co through raw:
driver=raw,file.driver=null-co,file.size=512
=> {"file": {}}
(Or something like that, I don't remember exactly.)
With some debugging sprinkled into block.c, I could see that the correct
options were there on the null-co level... But for some reason they
disappeared one level above.
Then I recalled that dict cloning is just a shallow cloning and looked
for the culprit...
Of course, in reality, much more cursing was involved.
Max
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] Restoring bitmaps after failed/cancelled migration
On 04/18/2018 10:00 AM, Vladimir Sementsov-Ogievskiy wrote: > Hi all. > > We now have the following problem: > > If dirty-bitmaps migration capability is enabled, persistance flag is > dropped for all migrated bitmaps, to prevent their storing to the > storage on inactivate. It works ok, persistence itself is migrated > through the migration channel. But on source, bitmaps becomes not > persistent, so if we, for some reasons, want to continue using source > vm, we'll lose bitmaps on stop/start. > Sorry for not following along more carefully, which kind of migration are we talking about in this case? > It's simple to fix it: just make bitmaps persistent again on invalidate > [1].. But I have some questions. > > 1. What are possible cases? I think about the following: > > a. migration cancel or fail, then invalidate > b. migration success, then qmp cont => invalidate > c. migration success, then stop/start (there was no invalidate, so [1] > will not work) > > > 2. Is it safe at all, saving bitmaps after inactivate, even without > persistence? > > Inactive disk implies, that it may be changed by somebody other, isn't > it? Is it possible, that target will change the disk, and then we return > control to the source? In this case bitmaps will be invalid. So, should > not we drop all the bitmaps on inactivate? >
Re: [Qemu-block] [PATCH 10/13] tests: Add QDict clone-flatten test
On 05/09/2018 11:55 AM, Max Reitz wrote: This new test verifies that qdict_flatten() does not modify a shallow clone of the given QDict. Signed-off-by: Max Reitz --- tests/check-qdict.c | 33 + 1 file changed, 33 insertions(+) I'm not sure I even want to know how long it took you to debug the crash that you obviously hit before adding the fix in 9/13 plus this test ;) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-block] [PATCH 09/13] qdict: Make qdict_flatten() shallow-clone-friendly
On 05/09/2018 11:55 AM, Max Reitz wrote:
In its current form, qdict_flatten() removes all entries from nested
QDicts that are moved to the root QDict. It is completely sufficient to
remove all old entries from the root QDict, however. If the nested
dicts have a refcount of 1, this will automatically delete them, too.
And if they have a greater refcount, we probably do not want to modify
them in the first place.
The latter observation means that it was currently (in general)
impossible to qdict_flatten() a shallowly cloned dict because that would
empty nested QDicts in the original dict as well. This patch changes
this, so you can now use qdict_flatten(qdict_shallow_clone(dict)) to get
a flattened copy without disturbing the original.
Signed-off-by: Max Reitz
---
qobject/qdict.c | 23 ---
1 file changed, 16 insertions(+), 7 deletions(-)
g_free(new_key);
-if (delete) {
+if (copied_value && qdict == target) {
+/* If we have copied a value, and if we are on the root
+ * level, we need to remove the old entry. Otherwise, we
+ * do not, because by removing these entries on the root
+ * level, the reference counts of nested dicts and listed
s/listed/lists/
+ * will be reduced automatically. In fact, we probably do
+ * not want to modify nested dicts and lists with
+ * refcounts greater than 1 anyway. */
qdict_del(qdict, entry->key);
-/* Restart loop after modifying the iterated QDict */
+/* Restart loop after modifying the iterated QDict. We
+ * only need to do this if qdict == target, because
+ * otherwise copying the value did not affect qdict. */
entry = qdict_first(qdict);
continue;
}
With the typo fix,
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [Qemu-block] [PATCH 07/13] qdict: Add qdict_stringify_for_keyval()
On 05/09/2018 11:55 AM, Max Reitz wrote:
The purpose of this function is to prepare a QDict for consumption by
the keyval visitor, which only accepts strings and QNull.
Signed-off-by: Max Reitz
---
include/block/qdict.h | 2 ++
qobject/qdict.c | 57 +++
2 files changed, 59 insertions(+)
+/**
+ * Convert all values in a QDict so it can be consumed by the keyval
+ * input visitor. QNull values are left as-is, all other values are
+ * converted to strings.
+ *
+ * @qdict must be flattened, i.e. it may not contain any nested QDicts
+ * or QLists.
Maybe s/flattened/flattened already/
or would it be worth letting this function PERFORM the flattening step
automatically?
+ */
+void qdict_stringify_for_keyval(QDict *qdict)
+{
+const QDictEntry *e;
+
+for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
+QString *new_value = NULL;
+
+switch (qobject_type(e->value)) {
+case QTYPE_QNULL:
+/* leave as-is */
+break;
+
+case QTYPE_QNUM: {
+char *str = qnum_to_string(qobject_to(QNum, e->value));
+new_value = qstring_from_str(str);
+g_free(str);
+break;
+}
+
+case QTYPE_QSTRING:
+/* leave as-is */
+break;
+
+case QTYPE_QDICT:
+case QTYPE_QLIST:
+/* @qdict must be flattened */
+abort();
Matches your documentation about requiring it to be already flattened.
+
+case QTYPE_QBOOL:
+if (qbool_get_bool(qobject_to(QBool, e->value))) {
+new_value = qstring_from_str("on");
+} else {
+new_value = qstring_from_str("off");
+}
+break;
+
+case QTYPE_NONE:
+case QTYPE__MAX:
+abort();
+}
+
+if (new_value) {
+QDictEntry *mut_e = (QDictEntry *)e;
A bit of a shame that you have to cast away const. It took me a couple
reads to figure out this meant 'mutable_e'. But in the end, the code
looks correct.
+qobject_unref(mut_e->value);
+mut_e->value = QOBJECT(new_value);
If we're okay requiring the caller to pre-flatten before calling this,
instead of offering to do it here,
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [Qemu-block] [PATCH 08/13] tests: Add qdict_stringify_for_keyval() test
On 05/11/2018 01:13 PM, Max Reitz wrote: + * "h": 0x, + * "i": true, + * "j": 0 Is it worth testing fun things like '-0.0'? Sure, why not. Maybe even infinity, although I'm not quite sure the input visitor can handle it... JSON can't handle Inf or NaN, even if the input visitor can. So probably best to not worry about those. + g_assert(!strcmp(qdict_get_str(dict, "a"), "null")); + g_assert(!strcmp(qdict_get_str(dict, "b"), "42")); + g_assert(!strcmp(qdict_get_str(dict, "c"), "-23")); + g_assert(!strcmp(qdict_get_str(dict, "d"), "off")); + g_assert(qobject_type(qdict_get(dict, "e")) == QTYPE_QNULL); Is it worth shortening this line to: g_assert(qobject_to(QNull, qdict_get(dict, "e"))); I think explicitly checking the type is a bit more expressive. Okay (qobject_to() checks the type under the hood, and returns non-NULL only when it was the right type - but I see your point about that being a bit more magic) -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-block] [PATCH 01/13] qapi: Add default-variant for flat unions
On 05/11/2018 12:59 PM, Max Reitz wrote:
On 2018-05-10 15:18, Eric Blake wrote:
On 05/10/2018 08:12 AM, Eric Blake wrote:
Oh, I just had a thought:
+++ b/scripts/qapi/visit.py
@@ -40,10 +40,20 @@ def gen_visit_object_members(name, base, members,
if variants:
+ if variants.default_tag_value is None:
+ ret += mcgen('''
+ %(c_name)s = obj->%(c_name)s;
+''',
+ c_name=c_name(variants.tag_member.name))
+ else:
+ ret += mcgen('''
+ if (obj->has_%(c_name)s) {
+ %(c_name)s = obj->%(c_name)s;
+ } else {
+ %(c_name)s = %(enum_const)s;
In this branch of code, is it worth also generating:
%has_(c_name)s = true;
You mean obj->has_%(c_name)s, and then also set obj->%(c_name)s?
Umm, yeah ;)
In fact, I think it is as simple as:
if variants:
if variants.default_tag_value:
ret += mcgen('''
if (!obj->has_%(c_name)s) {
obj->has_%(c_name)s = true;
obj->%(c_name)s = %(enum_const)s;
}
''')
ret += mcgen('''
switch (obj->%(c_name)s) {
...
and you are back to not needing a temporary variable.
That way, the rest of the C code does not have to check
has_discriminator, because the process of assigning the default will
ensure that has_discriminator is always true later on. It does have the
effect that output would never omit the discriminator - but that might
be a good thing: if we ever have an output union that used to have a
mandatory discriminator and want to now make it optional, we don't want
to break older clients that expected the discriminator to be present. It
does obscure whether input relied on the default, but I don't think that
hurts.
Also, it would make code a bit simpler because it can cover the !has_X
case under X == default_X.
But OTOH, you could make that case for any optional value -- except
where "is missing" has special value.
My preference - special-casing "is missing" is prone to abuse. I don't
want to support that if we can at all avoid it. The sane semantics is
that the default is populated as soon as we detect that something is
missing, and whether the user relies on the default by leaving the
discriminator absent, or explicitly supplies the discriminator set to
the default, the behavior should always be the same.
And that's the tricky part: I think it's hard to say that a missing
discriminator never has special meaning.
It won't, if we decide right now that we don't want to let it :)
We only need the
default-variant so we know which variant of the union to pick; but we
don't know that the fact that the discriminator value was missing does
not have special meaning.
Of course, we can simply foreclose that by setting it here.
And that's the way I'm leaning.
I don't have money in this game, so I suppose it's yours and Markus's
call. :-)
Markus, what's your preference?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [Qemu-block] [PATCH 08/13] tests: Add qdict_stringify_for_keyval() test
On 2018-05-10 18:02, Eric Blake wrote:
> On 05/09/2018 11:55 AM, Max Reitz wrote:
>> Signed-off-by: Max Reitz
>> ---
>> tests/check-qdict.c | 54
>> +
>> 1 file changed, 54 insertions(+)
>>
>
>> +static void qdict_stringify_for_keyval_test(void)
>> +{
>> + QDict *dict = qdict_new();
>> +
>> + /*
>> + * Test stringification of:
>> + *
>> + * {
>> + * "a": "null",
>> + * "b": 42,
>> + * "c": -23,
>> + * "d": false,
>> + * "e": null,
>> + * "f": "",
>> + * "g": 0.5,
>> + * "h": 0x,
>> + * "i": true,
>> + * "j": 0
>
> Is it worth testing fun things like '-0.0'?
Sure, why not. Maybe even infinity, although I'm not quite sure the
input visitor can handle it...
>> + g_assert(!strcmp(qdict_get_str(dict, "a"), "null"));
>> + g_assert(!strcmp(qdict_get_str(dict, "b"), "42"));
>> + g_assert(!strcmp(qdict_get_str(dict, "c"), "-23"));
>> + g_assert(!strcmp(qdict_get_str(dict, "d"), "off"));
>> + g_assert(qobject_type(qdict_get(dict, "e")) == QTYPE_QNULL);
>
> Is it worth shortening this line to:
> g_assert(qobject_to(QNull, qdict_get(dict, "e")));
I think explicitly checking the type is a bit more expressive.
Max
> Reviewed-by: Eric Blake
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH 06/13] block: Add block-specific QDict header
On 2018-05-10 16:54, Eric Blake wrote: > On 05/09/2018 11:55 AM, Max Reitz wrote: >> There are numerous QDict functions that have been introduced for and are >> used only by the block layer. Move their declarations into an own > > s/an own/their own/ > >> header file to reflect that. >> >> While qdict_extract_subqdict() is in fact used outside of the block >> layer (in util/qemu-config.c), it is still a function related very >> closely to how the block layer works with nested QDicts, namely by >> sometimes flattening them. Therefore, its declaration is put into this >> header as well and util/qemu-config.c includes it with a comment stating >> exactly which function it needs. >> >> Suggested-by: Markus Armbruster >> Signed-off-by: Max Reitz >> --- > >> +++ b/include/block/qdict.h >> @@ -0,0 +1,35 @@ >> +/* >> + * Special QDict functions used by the block layer >> + * >> + * Copyright (c) 2018 Red Hat, Inc. > > You are extracting this from qdict.h which has: > * Copyright (C) 2009 Red Hat Inc. > > Is it worth listing 2009-2018, instead of just this year? I don't know, is it? I don't know whether it makes a real difference. >> + >> +void qdict_copy_default(QDict *dst, QDict *src, const char *key); >> +void qdict_set_default_str(QDict *dst, const char *key, const char >> *val); > > These two might be useful outside of the block layer; we can move them > back in a later patch if so. But for now, I agree with your choice of > moving them. > >> + >> +void qdict_join(QDict *dest, QDict *src, bool overwrite); > > This is borderline whether it would be useful outside of the block layer. I decided I wanted to move the *_default* functions, and if I did that, I would have to move this one as well. I decided I can't be biased just because I wrote this one. :-) But in general I'd say if anyone wants to use any of these functions outside of the block layer, they are welcome to move them back to the main header, provided they have a good reason to do so. I suppose a good reason for using qdict_join() may indeed turn up sooner or later. Max >> + >> +void qdict_extract_subqdict(QDict *src, QDict **dst, const char *start); >> +void qdict_array_split(QDict *src, QList **dst); >> +int qdict_array_entries(QDict *src, const char *subqdict); >> +QObject *qdict_crumple(const QDict *src, Error **errp); >> +void qdict_flatten(QDict *qdict); > > And these are definitely hacks that the block layer relies on, where > hopefully someday long term we can rewrite the block layer to use QAPI > types directly instead of constant reconversion between QemuOpts and > QDict and QAPI types. > > Reviewed-by: Eric Blake > signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH 05/13] qapi: Formalize qcow encryption probing
On 2018-05-10 16:24, Eric Blake wrote:
> On 05/09/2018 11:55 AM, Max Reitz wrote:
>> Currently, you can give no encryption format for a qcow file while still
>> passing a key-secret. That does not conform to the schema, so this
>> patch changes the schema to allow it.
>>
>> Signed-off-by: Max Reitz
>> ---
>
>> ##
>> # @BlockdevQcowEncryptionFormat:
>> #
>> # @aes: AES-CBC with plain64 initialization vectors
>> #
>> +# @from-image: Determine the encryption format from the image
>> +# header. This only allows the use of the
>> +# key-secret option. (Since: 2.13)
>> +#
>> # Since: 2.10
>> ##
>> { 'enum': 'BlockdevQcowEncryptionFormat',
>> - 'data': [ 'aes' ] }
>> + 'data': [ 'aes', 'from-image' ] }
>
> Overkill. Why not just:
>
>> ##
>> # @BlockdevQcowEncryption:
>> @@ -2728,9 +2748,11 @@
>> # Since: 2.10
>> ##
>> { 'union': 'BlockdevQcowEncryption',
>> - 'base': { 'format': 'BlockdevQcowEncryptionFormat' },
>> + 'base': { '*format': 'BlockdevQcowEncryptionFormat' },
>> 'discriminator': 'format',
>> - 'data': { 'aes': 'QCryptoBlockOptionsQCow' } }
>> + 'default-variant': 'from-image',
>
> 'default-variant': 'aes'
>
>> + 'data': { 'aes': 'QCryptoBlockOptionsQCow',
>
> and call it good, because there are no other options to pick from, so
> 'from-image' would always resolve to 'aes' anyway.
H. Yes. :-)
Max
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH 03/13] tests: Add QAPI optional discriminator tests
On 2018-05-10 16:08, Eric Blake wrote:
> On 05/09/2018 11:55 AM, Max Reitz wrote:
>> There already is an optional discriminator test, although it also noted
>> the discriminator name itself as optional. Changing that, and adding a
>> default-variant field, makes that test pass instead of failing.
>
> I'm not sure I agree with that one. I think that you should instead add
> a positive test of a default variant into qapi-schema-test.json,
> especially since that is the one positive test where we also ensure that
> the C compiler is happy with the generated code (other positive tests
> prove that the generator produced something without error, but not that
> what it produced could be compiled).
Oh, sure.
>> 17 files changed, 61 insertions(+), 4 deletions(-)
>> create mode 100644
>> tests/qapi-schema/flat-union-optional-discriminator-invalid-default.json
>> create mode 100644
>> tests/qapi-schema/flat-union-optional-discriminator-no-default.json
>> create mode 100644
>> tests/qapi-schema/flat-union-superfluous-default-variant.json
>> create mode 100644
>> tests/qapi-schema/flat-union-optional-discriminator-invalid-default.err
>> create mode 100644
>> tests/qapi-schema/flat-union-optional-discriminator-invalid-default.exit
>> create mode 100644
>> tests/qapi-schema/flat-union-optional-discriminator-invalid-default.out
>> create mode 100644
>> tests/qapi-schema/flat-union-optional-discriminator-no-default.err
>> create mode 100644
>> tests/qapi-schema/flat-union-optional-discriminator-no-default.exit
>> create mode 100644
>> tests/qapi-schema/flat-union-optional-discriminator-no-default.out
>> create mode 100644
>> tests/qapi-schema/flat-union-superfluous-default-variant.err
>> create mode 100644
>> tests/qapi-schema/flat-union-superfluous-default-variant.exit
>> create mode 100644
>> tests/qapi-schema/flat-union-superfluous-default-variant.out
>
> Patch 1 converted 2 error messages into 6, where 2 of them look identical:
>
>>> - # The value of member 'discriminator' must name a non-optional
>>> - # member of the base struct.
>>> + # The value of member 'discriminator' must name a member of
>>> + # the base struct.
>>> check_name(info, "Discriminator of flat union '%s'" % name,
>>> discriminator)
>
> [0] check_name() checks that 'discriminator':'*name' is rejected - this
> check is unchanged
>
>>> - discriminator_type = base_members.get(discriminator)
>>> - if not discriminator_type:
>>> - raise QAPISemError(info,
>>> - "Discriminator '%s' is not a member
>>> of base "
>>> - "struct '%s'"
>>> - % (discriminator, base))
>
> The old code ensured that 'discriminator':'name' finds 'name' as a
> mandatory field in the base type; which changed into:
>
>>> + if default_variant is None:
>>> + discriminator_type = base_members.get(discriminator)
>>> + if not discriminator_type:
>>> + if base_members.get('*' + discriminator) is None:
>>> + raise QAPISemError(info,
>>> + "Discriminator '%s' is not a
>>> member of "
>>> + "base struct '%s'"
>>> + % (discriminator, base))
>
> [1] the discriminator type must be a member field (we didn't find either
> a mandatory or an optional field)
>
>>> + else:
>>> + raise QAPISemError(info,
>>> + "Default variant must be
>>> specified for "
>>> + "optional discriminator '%s'"
>>> + % discriminator)
>
> [2] without default_variant, the member field must be mandatory
>
>>> + else:
>>> + discriminator_type = base_members.get('*' + discriminator)
>>> + if not discriminator_type:
>>> + if base_members.get(discriminator) is None:
>>> + raise QAPISemError(info,
>>> + "Discriminator '%s' is not a
>>> member of "
>>> + "base struct '%s'"
>>> + % (discriminator, base))
>
> [3] the discriminator type must be a member field (we didn't find either
> a mandatory or an optional field), identical message to [1]
>
>>> + else:
>>> + raise QAPISemError(info,
>>> + "Must not specify a default
>>> variant for "
>>> + "non-optional discriminator
>>> '%s'"
>>> + % discriminator)
>
> [4] with default_variant, the member field must be optional
>
>>> +
>>> enum_define = enum_types.get(discriminator_type)
>>> allow_metas = ['struct']
>>>
Re: [Qemu-block] [PATCH 01/13] qapi: Add default-variant for flat unions
On 2018-05-10 15:18, Eric Blake wrote:
> On 05/10/2018 08:12 AM, Eric Blake wrote:
>
> Oh, I just had a thought:
>
>>> +++ b/scripts/qapi/visit.py
>>> @@ -40,10 +40,20 @@ def gen_visit_object_members(name, base, members,
>
>>> if variants:
>>> + if variants.default_tag_value is None:
>>> + ret += mcgen('''
>>> + %(c_name)s = obj->%(c_name)s;
>>> +''',
>>> + c_name=c_name(variants.tag_member.name))
>>> + else:
>>> + ret += mcgen('''
>>> + if (obj->has_%(c_name)s) {
>>> + %(c_name)s = obj->%(c_name)s;
>>> + } else {
>>> + %(c_name)s = %(enum_const)s;
>
> In this branch of code, is it worth also generating:
>
> %has_(c_name)s = true;
You mean obj->has_%(c_name)s, and then also set obj->%(c_name)s?
> That way, the rest of the C code does not have to check
> has_discriminator, because the process of assigning the default will
> ensure that has_discriminator is always true later on. It does have the
> effect that output would never omit the discriminator - but that might
> be a good thing: if we ever have an output union that used to have a
> mandatory discriminator and want to now make it optional, we don't want
> to break older clients that expected the discriminator to be present. It
> does obscure whether input relied on the default, but I don't think that
> hurts.
Also, it would make code a bit simpler because it can cover the !has_X
case under X == default_X.
But OTOH, you could make that case for any optional value -- except
where "is missing" has special value.
And that's the tricky part: I think it's hard to say that a missing
discriminator never has special meaning. We only need the
default-variant so we know which variant of the union to pick; but we
don't know that the fact that the discriminator value was missing does
not have special meaning.
Of course, we can simply foreclose that by setting it here.
I don't have money in this game, so I suppose it's yours and Markus's
call. :-)
Max
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH 01/13] qapi: Add default-variant for flat unions
On 2018-05-10 15:12, Eric Blake wrote:
> On 05/09/2018 11:55 AM, Max Reitz wrote:
>> This patch allows specifying a discriminator that is an optional member
>> of the base struct. In such a case, a default value must be provided
>> that is used when no value is given.
>>
>> Signed-off-by: Max Reitz
>> ---
>> qapi/introspect.json | 8 ++
>> scripts/qapi/common.py | 57
>> ++
>> scripts/qapi/doc.py | 8 --
>> scripts/qapi/introspect.py | 10 +---
>> scripts/qapi/visit.py | 33 ++--
>> tests/qapi-schema/test-qapi.py | 2 ++
>> 6 files changed, 101 insertions(+), 17 deletions(-)
>
> I've been threatening that we might need this for some time, so I'm glad
> to see it being implemented. We'll see if the tests in 2 and 3 cover
> the code added here.
>
>>
>> diff --git a/qapi/introspect.json b/qapi/introspect.json
>> index c7f67b7d78..2d7b1e3745 100644
>> --- a/qapi/introspect.json
>> +++ b/qapi/introspect.json
>> @@ -168,6 +168,13 @@
>> # @tag: the name of the member serving as type tag.
>> # An element of @members with this name must exist.
>> #
>> +# @default-variant: if the @tag element of @members is optional, this
>> +# is the default value for choosing a variant. Its
>> +# value must be a valid value for @tag.
>
> Perhaps s/must/will/ as this struct is used for output (and therefore we
> always meet the condition, rather than the user having to do something
> correctly).
I mostly copied from the other descriptions which seemed to prefer
"must", but I'm happy with either.
> Nice that you remembered introspection.
I didn't, because I had no idea how introspection works exactly before
this series. But one of the tests broke, thus telling me I might have
forgotten something. :-)
>> +# Present exactly when @tag is present and the
>> +# associated element of @members is optional.
>> +# (Since: 2.13)
>> +#
>> # @variants: variant members, i.e. additional members that
>> # depend on the type tag's value. Present exactly when
>> # @tag is present. The variants are in no particular order,
[...]
>> diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
>> index 5d72d8936c..ecffc46bd3 100644
>> --- a/scripts/qapi/visit.py
>> +++ b/scripts/qapi/visit.py
>> @@ -40,10 +40,20 @@ def gen_visit_object_members(name, base, members,
>> variants):
>> void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj,
>> Error **errp)
>> {
>> Error *err = NULL;
>> -
>> ''',
>> c_name=c_name(name))
>> + if variants:
>> + ret += mcgen('''
>> + %(c_type)s %(c_name)s;
>> +''',
>> + c_type=variants.tag_member.type.c_name(),
>> + c_name=c_name(variants.tag_member.name))
>> +
>> + ret += mcgen('''
>> +
>> +''')
>> +
>
> Creating a temp variable makes it easier to handle the default...
>
>> if base:
>> ret += mcgen('''
>> visit_type_%(c_type)s_members(v, (%(c_type)s *)obj, &err);
>> @@ -75,8 +85,27 @@ void visit_type_%(c_name)s_members(Visitor *v,
>> %(c_name)s *obj, Error **errp)
>> ''')
>> if variants:
>> + if variants.default_tag_value is None:
>> + ret += mcgen('''
>> + %(c_name)s = obj->%(c_name)s;
>> +''',
>> + c_name=c_name(variants.tag_member.name))
>> + else:
>> + ret += mcgen('''
>> + if (obj->has_%(c_name)s) {
>> + %(c_name)s = obj->%(c_name)s;
>> + } else {
>> + %(c_name)s = %(enum_const)s;
>> + }
>> +''',
>> + c_name=c_name(variants.tag_member.name),
>> + enum_const=c_enum_const(
>> + variants.tag_member.type.name,
>> + variants.default_tag_value,
>> + variants.tag_member.type.prefix))
>> +
>> ret += mcgen('''
>> - switch (obj->%(c_name)s) {
>> + switch (%(c_name)s) {
>
> ...compared to the old code that just inlined the one thing that used to
> be assigned to what is now the temporary variable.
>
> It might be possible to inline things so that the generated code reads
> either:
>
> switch (obj->discriminator) {
> switch (!obj->has_discriminator ? DEFAULT : obj->discriminator) {
>
> but I don't think it's worth the effort; and the temporary variable is
> fine even though it makes the generated file bigger.
I don't really mind either way, but depending on the default value and
the discriminator name, using ?: may lead to a rather long line.
>> ''',
>> c_name=c_name(variants.tag_member.name))
>> diff --git a/tests/qapi-schema/test-qapi.py
>> b/tests/qapi-schema/test-qapi.py
>> index c1a144ba29..f2a072b92e 100644
>> --- a/tests/qapi-schema/test-qapi.py
>> +++ b/tests/qapi-schema
Re: [Qemu-block] [Qemu-devel] [PATCH 04/13] qapi: Formalize qcow2 encryption probing
On 2018-05-10 09:58, Daniel P. Berrangé wrote:
> On Wed, May 09, 2018 at 06:55:21PM +0200, Max Reitz wrote:
>> Currently, you can give no encryption format for a qcow2 file while
>> still passing a key-secret. That does not conform to the schema, so
>> this patch changes the schema to allow it.
>>
>> Signed-off-by: Max Reitz
>> ---
>> qapi/block-core.json | 44
>> 1 file changed, 40 insertions(+), 4 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 71c9ab8538..092a1aba2d 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -43,6 +43,19 @@
>> { 'struct': 'ImageInfoSpecificQCow2EncryptionBase',
>>'data': { 'format': 'BlockdevQcow2EncryptionFormat'}}
>>
>> +##
>> +# @ImageInfoSpecificQCow2EncryptionNoInfo:
>> +#
>> +# Only used for the qcow2 encryption format "from-image" in which the
>> +# actual encryption format is determined from the image header.
>> +# Therefore, this encryption format will never be reported in
>> +# ImageInfoSpecificQCow2Encryption.
>> +#
>> +# Since: 2.13
>> +##
>> +{ 'struct': 'ImageInfoSpecificQCow2EncryptionNoInfo',
>> + 'data': { } }
>> +
>> ##
>> # @ImageInfoSpecificQCow2Encryption:
>> #
>> @@ -52,7 +65,8 @@
>>'base': 'ImageInfoSpecificQCow2EncryptionBase',
>>'discriminator': 'format',
>>'data': { 'aes': 'QCryptoBlockInfoQCow',
>> -'luks': 'QCryptoBlockInfoLUKS' } }
>> +'luks': 'QCryptoBlockInfoLUKS',
>> +'from-image': 'ImageInfoSpecificQCow2EncryptionNoInfo' } }
>>
>> ##
>> # @ImageInfoSpecificQCow2:
>> @@ -2739,10 +2753,30 @@
>> # @BlockdevQcow2EncryptionFormat:
>> # @aes: AES-CBC with plain64 initialization venctors
>> #
>> +# @from-image: Determine the encryption format from the image
>> +# header. This only allows the use of the
>> +# key-secret option. (Since: 2.13)
>> +#
>> # Since: 2.10
>> ##
>> { 'enum': 'BlockdevQcow2EncryptionFormat',
>> - 'data': [ 'aes', 'luks' ] }
>> + 'data': [ 'aes', 'luks', 'from-image' ] }
>> +
>> +##
>> +# @BlockdevQcow2EncryptionSecret:
>> +#
>> +# Allows specifying a key-secret without specifying the exact
>> +# encryption format, which is determined automatically from the image
>> +# header.
>> +#
>> +# @key-secret: The ID of a QCryptoSecret object providing the
>> +# decryption key. Mandatory except when probing
>> +# image for metadata only.
>> +#
>> +# Since: 2.13
>> +##
>> +{ 'struct': 'BlockdevQcow2EncryptionSecret',
>> + 'data': { '*key-secret': 'str' } }
>>
>> ##
>> # @BlockdevQcow2Encryption:
>> @@ -2750,10 +2784,12 @@
>> # Since: 2.10
>> ##
>> { 'union': 'BlockdevQcow2Encryption',
>> - 'base': { 'format': 'BlockdevQcow2EncryptionFormat' },
>> + 'base': { '*format': 'BlockdevQcow2EncryptionFormat' },
>>'discriminator': 'format',
>> + 'default-variant': 'from-image',
>>'data': { 'aes': 'QCryptoBlockOptionsQCow',
>> -'luks': 'QCryptoBlockOptionsLUKS'} }
>> +'luks': 'QCryptoBlockOptionsLUKS',
>> +'from-image': 'BlockdevQcow2EncryptionSecret' } }
>
> Bike-shedding on name, how about "auto" or "probe" ?
Sure. I like "probe" a bit better than "auto", although "auto" is what
we usually have... But I think "probe" is still a bit better.
>
> IIUC, this schema addition means the QAPI parser now allows
>
>encrypt.format=from-image,encrypt.key-secret=sec0,...other opts...
>
> but the code will not accept "from-image" as a valid string.
Ah, right, I forgot that. Will fix.
Thanks for reviewing!
Max
> eg qcow2_update_options_prepare() will do
>
> case QCOW_CRYPT_AES:
> if (encryptfmt && !g_str_equal(encryptfmt, "aes")) {
> error_setg(errp,
>"Header reported 'aes' encryption format but "
>"options specify '%s'", encryptfmt);
> ret = -EINVAL;
> goto fail;
> }
>
>...snip
>
> case QCOW_CRYPT_LUKS:
> if (encryptfmt && !g_str_equal(encryptfmt, "luks")) {
> error_setg(errp,
>"Header reported 'luks' encryption format but "
>"options specify '%s'", encryptfmt);
> ret = -EINVAL;
> goto fail;
> }
>
>
> Regards,
> Daniel
>
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v3] qcow2: fix preallocation with metadata on bare block device
Am 11.05.2018 um 17:36 hat Ivan Ren geschrieben: > Create a qcow2 directly on bare block device with > "-o preallocation=metadata" option. When read this qcow2, it will > return pre-existing data on block device. This patch add > QCOW_OFLAG_ZERO flag (supported in qcow_version >= 3) for > preallocated l2 entry to avoid this problem. > > Signed-off-by: Ivan Ren Doesn't this defeat the purpose of preallocation? Usually, the idea with preallocation is that you don't need to update any metadata on the first write, but if you set QCOW_OFLAG_ZERO, we do need a metadata update again. So what's the advantage compared to not preallocating at all? Kevin
Re: [Qemu-block] [Qemu-devel] Some question about savem/qcow2 incremental snapshot
Am 10.05.2018 um 10:26 hat Stefan Hajnoczi geschrieben: > On Wed, May 09, 2018 at 07:54:31PM +0200, Max Reitz wrote: > > On 2018-05-09 12:16, Stefan Hajnoczi wrote: > > > On Tue, May 08, 2018 at 05:03:09PM +0200, Kevin Wolf wrote: > > >> Am 08.05.2018 um 16:41 hat Eric Blake geschrieben: > > >>> On 12/25/2017 01:33 AM, He Junyan wrote: > > >> 2. Make the nvdimm device use the QEMU block layer so that it is backed > > >>by a non-raw disk image (such as a qcow2 file representing the > > >>content of the nvdimm) that supports snapshots. > > >> > > >>This part is hard because it requires some completely new > > >>infrastructure such as mapping clusters of the image file to guest > > >>pages, and doing cluster allocation (including the copy on write > > >>logic) by handling guest page faults. > > >> > > >> I think it makes sense to invest some effort into such interfaces, but > > >> be prepared for a long journey. > > > > > > I like the suggestion but it needs to be followed up with a concrete > > > design that is feasible and fair for Junyan and others to implement. > > > Otherwise the "long journey" is really just a way of rejecting this > > > feature. > > > > > > Let's discuss the details of using the block layer for NVDIMM and try to > > > come up with a plan. > > > > > > The biggest issue with using the block layer is that persistent memory > > > applications use load/store instructions to directly access data. This > > > is fundamentally different from the block layer, which transfers blocks > > > of data to and from the device. > > > > > > Because of block DMA, QEMU is able to perform processing at each block > > > driver graph node. This doesn't exist for persistent memory because > > > software does not trap I/O. Therefore the concept of filter nodes > > > doesn't make sense for persistent memory - we certainly do not want to > > > trap every I/O because performance would be terrible. > > > > > > Another difference is that persistent memory I/O is synchronous. > > > Load/store instructions execute quickly. Perhaps we could use KVM async > > > page faults in cases where QEMU needs to perform processing, but again > > > the performance would be bad. > > > > Let me first say that I have no idea how the interface to NVDIMM looks. > > I just assume it works pretty much like normal RAM (so the interface is > > just that it’s a part of the physical address space). > > > > Also, it sounds a bit like you are already discarding my idea, but here > > goes anyway. > > > > Would it be possible to introduce a buffering block driver that presents > > the guest an area of RAM/NVDIMM through an NVDIMM interface (so I > > suppose as part of the guest address space)? For writing, we’d keep a > > dirty bitmap on it, and then we’d asynchronously move the dirty areas > > through the block layer, so basically like mirror. On flushing, we’d > > block until everything is clean. > > > > For reading, we’d follow a COR/stream model, basically, where everything > > is unpopulated in the beginning and everything is loaded through the > > block layer both asynchronously all the time and on-demand whenever the > > guest needs something that has not been loaded yet. > > > > Now I notice that that looks pretty much like a backing file model where > > we constantly run both a stream and a commit job at the same time. > > > > The user could decide how much memory to use for the buffer, so it could > > either hold everything or be partially unallocated. > > > > You’d probably want to back the buffer by NVDIMM normally, so that > > nothing is lost on crashes (though this would imply that for partial > > allocation the buffering block driver would need to know the mapping > > between the area in real NVDIMM and its virtual representation of it). > > > > Just my two cents while scanning through qemu-block to find emails that > > don’t actually concern me... > > The guest kernel already implements this - it's the page cache and the > block layer! > > Doing it in QEMU with dirty memory logging enabled is less efficient > than doing it in the guest. > > That's why I said it's better to just use block devices than to > implement buffering. > > I'm saying that persistent memory emulation on top of the iscsi:// block > driver (for example) does not make sense. It could be implemented but > the performance wouldn't be better than block I/O and the > complexity/code size in QEMU isn't justified IMO. I think it could make sense if you put everything together. The primary motivation to use this would of course be that you can directly map the guest clusters of a qcow2 file into the guest. We'd potentially fault on the first access, but once it's mapped, you get raw speed. You're right about flushing, and I was indeed thinking of Pankaj's work there; maybe I should have been more explicit about that. Now buffering in QEMU might come in useful when you want to run a block job on the device. Block jobs a
Re: [Qemu-block] [PATCH v2 0/2] block/file-posix: allow -drive cache.direct=off live migration
On Fri, Apr 27, 2018 at 05:23:10PM +0100, Stefan Hajnoczi wrote: > v2: > * Add comment on !__linux__ situation [Fam] > * Add file-posix.c x-check-cache-dropped=on|off option [DaveG, Kevin] > > file-posix.c only supports shared storage live migration with -drive > cache.direct=off due to cache consistency issues. There are two main shared > storage configurations: files on NFS and host block devices on SAN LUNs. > > The problem is that QEMU starts on the destination host before the source host > has written everything out to the disk. The page cache on the destination > host > may contain stale data read when QEMU opened the image file (before migration > handover). Using O_DIRECT avoids this problem but prevents users from taking > advantage of the host page cache. > > Although cache=none is the recommended setting for virtualization use cases, > there are scenarios where cache=writeback makes sense. If the guest has much > less RAM than the host or many guests share the same backing file, then the > host page cache can significantly improve disk I/O performance. > > This patch series implements .bdrv_co_invalidate_cache() for > block/file-posix.c > on Linux so that shared storage live migration works. I have sent it as an > RFC > because cache consistency is not binary, there are corner cases which I've > described in the actual patch, and this may require more discussion. > > Regarding NFS, QEMU relies on O_DIRECT rather than the close-to-open > consistency model (see nfs(5)), which is the basic guarantee provided by NFS. > After this patch cache consistency is no longer provided by O_DIRECT. > > This patch series relies on fdatasync(2) (source) + > posix_fadvise(POSIX_FADV_DONTNEED) (destination) instead. I believe it is > safe > for both NFS and SAN LUNs. Maybe we should use fsync(2) instead of > fdatasync(2) so that NFS has up-to-date inode metadata? > > Stefan Hajnoczi (2): > block/file-posix: implement bdrv_co_invalidate_cache() on Linux > block/file-posix: add x-check-page-cache=on|off option > > qapi/block-core.json | 7 ++- > block/file-posix.c | 146 > ++- > 2 files changed, 150 insertions(+), 3 deletions(-) > > -- > 2.14.3 > Thanks, applied to my block tree: https://github.com/stefanha/qemu/commits/block Stefan signature.asc Description: PGP signature
[Qemu-block] [PATCH v3] qcow2: fix preallocation with metadata on bare block device
Create a qcow2 directly on bare block device with
"-o preallocation=metadata" option. When read this qcow2, it will
return pre-existing data on block device. This patch add
QCOW_OFLAG_ZERO flag (supported in qcow_version >= 3) for
preallocated l2 entry to avoid this problem.
Signed-off-by: Ivan Ren
---
Changes in v2:
- always pass QCOW_OFLAG_ZERO when preallocate metadta
Changes in v3:
- limit this feature only on qcow_version >= 3
---
block/qcow2-cluster.c | 5 +++--
block/qcow2.c | 12 +---
block/qcow2.h | 3 ++-
3 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 1aee726..b9e0ceb 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -919,7 +919,8 @@ fail:
return ret;
}
-int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m)
+int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m,
+uint64_t flags)
{
BDRVQcow2State *s = bs->opaque;
int i, j = 0, l2_index, ret;
@@ -969,7 +970,7 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs,
QCowL2Meta *m)
}
l2_slice[l2_index + i] = cpu_to_be64((cluster_offset +
-(i << s->cluster_bits)) | QCOW_OFLAG_COPIED);
+(i << s->cluster_bits)) | QCOW_OFLAG_COPIED | flags);
}
diff --git a/block/qcow2.c b/block/qcow2.c
index 2f36e63..ee862b0 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2044,7 +2044,7 @@ static coroutine_fn int qcow2_co_pwritev(BlockDriverState
*bs, uint64_t offset,
while (l2meta != NULL) {
QCowL2Meta *next;
-ret = qcow2_alloc_cluster_link_l2(bs, l2meta);
+ret = qcow2_alloc_cluster_link_l2(bs, l2meta, 0);
if (ret < 0) {
goto fail;
}
@@ -2552,7 +2552,13 @@ static void coroutine_fn preallocate_co(void *opaque)
while (meta) {
QCowL2Meta *next = meta->next;
-ret = qcow2_alloc_cluster_link_l2(bs, meta);
+if (s->qcow_version >= 3) {
+/* add QCOW_OFLAG_ZERO to avoid pre-existing data be read */
+ret = qcow2_alloc_cluster_link_l2(bs, meta, QCOW_OFLAG_ZERO);
+} else {
+ret = qcow2_alloc_cluster_link_l2(bs, meta, 0);
+}
+
if (ret < 0) {
qcow2_free_any_clusters(bs, meta->alloc_offset,
meta->nb_clusters,
QCOW2_DISCARD_NEVER);
@@ -3458,7 +3464,7 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t
offset,
};
qemu_co_queue_init(&allocation.dependent_requests);
-ret = qcow2_alloc_cluster_link_l2(bs, &allocation);
+ret = qcow2_alloc_cluster_link_l2(bs, &allocation, 0);
if (ret < 0) {
error_setg_errno(errp, -ret, "Failed to update L2 tables");
qcow2_free_clusters(bs, host_offset,
diff --git a/block/qcow2.h b/block/qcow2.h
index adf5c39..9a59602 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -617,7 +617,8 @@ uint64_t
qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,
uint64_t offset,
int compressed_size);
-int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m);
+int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m,
+uint64_t flags);
int qcow2_cluster_discard(BlockDriverState *bs, uint64_t offset,
uint64_t bytes, enum qcow2_discard_type type,
bool full_discard);
--
1.8.3.1
Re: [Qemu-block] [Qemu-devel] [PATCH v2] qcow2: fix preallocation with metadata on bare block device
>> Create a qcow2 directly on bare block device with >> "-o preallocation=metadata" option. When read this qcow2, it will >> return pre-existing data on block device, and this may lead to >> data leakage. This patch add QCOW_OFLAG_ZERO for all preallocated >> l2 entry to avoid this problem. > > This is a semantic change; are we okay making it? > Does your code properly check for qcow2v2 files, which don't support > QCOW_OFLAG_ZERO (only qcow2v3 supports it)? Sorry for this mistake. Current solution can only be used with s->qcow_version >= 3. I'll fix it. On Fri, May 11, 2018 at 9:41 PM Eric Blake wrote: > On 05/11/2018 07:37 AM, Ivan Ren wrote: > > Create a qcow2 directly on bare block device with > > "-o preallocation=metadata" option. When read this qcow2, it will > > return pre-existing data on block device, and this may lead to > > data leakage. This patch add QCOW_OFLAG_ZERO for all preallocated > > l2 entry to avoid this problem. > > This is a semantic change; are we okay making it? > > Does your code properly check for qcow2v2 files, which don't support > QCOW_OFLAG_ZERO (only qcow2v3 supports it)? > > > > > Signed-off-by: Ivan Ren > > --- > > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3266 > Virtualization: qemu.org | libvirt.org >
Re: [Qemu-block] [PATCH v4 02/10] raw: Check byte range uniformly
On 05/11/2018 07:08 AM, Fam Zheng wrote:
We don't verify the request range against s->size in the I/O callbacks
except for raw_co_pwritev. This is wrong (especially for
raw_co_pwrite_zeroes and raw_co_pdiscard), so fix them.
Did you bother identifying how long the bug has been present (but read
below, because I'm not sure there was even a bug)?
CC: qemu-sta...@nongnu.org
Signed-off-by: Fam Zheng
---
block/raw-format.c | 63 --
1 file changed, 38 insertions(+), 25 deletions(-)
diff --git a/block/raw-format.c b/block/raw-format.c
index a378547c99..803083f1a1 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -167,16 +167,36 @@ static void raw_reopen_abort(BDRVReopenState *state)
state->opaque = NULL;
}
+/* Check and adjust the offset, against 'offset' and 'size' options. */
+static inline int raw_adjust_offset(BlockDriverState *bs, uint64_t *offset,
+uint64_t bytes)
+{
+BDRVRawState *s = bs->opaque;
+
+if (s->has_size && (*offset > s->size || bytes > (s->size - *offset))) {
+/* There's not enough space for the data. Don't write anything and just
+ * fail to prevent leaking out of the size specified in options. */
+return -ENOSPC;
+}
Can this even trigger? The block layer should already be clamping
requests according to the device's reported size, and we already report
a smaller size according to s->size and s->offset. This could probably
be an assertion instead.
+
+if (*offset > UINT64_MAX - s->offset) {
+return -EINVAL;
Should this be against INT64_MAX instead? After all, we really do use
off_t (a 63-bit quantity, since it is signed), rather than uint64_t, as
our maximum (theoretical) image size. But again, can it even trigger,
or can it be an assertion?
+}
+*offset += s->offset;
+
+return 0;
+}
+
static int coroutine_fn raw_co_preadv(BlockDriverState *bs, uint64_t offset,
uint64_t bytes, QEMUIOVector *qiov,
int flags)
{
-BDRVRawState *s = bs->opaque;
+int ret;
-if (offset > UINT64_MAX - s->offset) {
-return -EINVAL;
+ret = raw_adjust_offset(bs, &offset, bytes);
If I'm right and we can assert instead of failing, then
raw_adjust_offset() doesn't return failure. If I'm wrong, then there is
now a code path where we can return ENOSPC on a read, which is unusual
and probably wrong.
+if (ret) {
+return ret;
}
-offset += s->offset;
BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO);
return bdrv_co_preadv(bs->file, offset, bytes, qiov, flags);
@@ -186,23 +206,11 @@ static int coroutine_fn raw_co_pwritev(BlockDriverState
*bs, uint64_t offset,
uint64_t bytes, QEMUIOVector *qiov,
int flags)
{
-BDRVRawState *s = bs->opaque;
void *buf = NULL;
BlockDriver *drv;
QEMUIOVector local_qiov;
int ret;
-if (s->has_size && (offset > s->size || bytes > (s->size - offset))) {
-/* There's not enough space for the data. Don't write anything and just
- * fail to prevent leaking out of the size specified in options. */
-return -ENOSPC;
-}
-
-if (offset > UINT64_MAX - s->offset) {
-ret = -EINVAL;
-goto fail;
-}
Okay, so you're just doing code refactoring; perhaps we could have done
assertions here.
-
if (bs->probed && offset < BLOCK_PROBE_BUF_SIZE && bytes) {
/* Handling partial writes would be a pain - so we just
* require that guests have 512-byte request alignment if
@@ -237,7 +245,10 @@ static int coroutine_fn raw_co_pwritev(BlockDriverState
*bs, uint64_t offset,
qiov = &local_qiov;
}
-offset += s->offset;
+ret = raw_adjust_offset(bs, &offset, bytes);
+if (ret) {
+goto fail;
+}
BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO);
ret = bdrv_co_pwritev(bs->file, offset, bytes, qiov, flags);
@@ -267,22 +278,24 @@ static int coroutine_fn
raw_co_pwrite_zeroes(BlockDriverState *bs,
int64_t offset, int bytes,
BdrvRequestFlags flags)
{
-BDRVRawState *s = bs->opaque;
-if (offset > UINT64_MAX - s->offset) {
-return -EINVAL;
+int ret;
+
+ret = raw_adjust_offset(bs, (uint64_t *)&offset, bytes);
+if (ret) {
+return ret;
}
-offset += s->offset;
If I'm right and raw_adjust_offset() can't fail, then this didn't add
any protection. If I'm wrong and it is possible to get the block layer
to send a request beyond our advertised size, then this is indeed a bug
fix worthy of being on the stable branch.
return bdrv_co_pwrite_zeroes(bs->file, offset, bytes, flags);
}
static int coroutine_fn raw_co_
Re: [Qemu-block] [Qemu-devel] [PATCH v2] qcow2: fix preallocation with metadata on bare block device
On 05/11/2018 07:37 AM, Ivan Ren wrote: Create a qcow2 directly on bare block device with "-o preallocation=metadata" option. When read this qcow2, it will return pre-existing data on block device, and this may lead to data leakage. This patch add QCOW_OFLAG_ZERO for all preallocated l2 entry to avoid this problem. This is a semantic change; are we okay making it? Does your code properly check for qcow2v2 files, which don't support QCOW_OFLAG_ZERO (only qcow2v3 supports it)? Signed-off-by: Ivan Ren --- -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
[Qemu-block] [PATCH v2] qcow2: fix preallocation with metadata on bare block device
Create a qcow2 directly on bare block device with
"-o preallocation=metadata" option. When read this qcow2, it will
return pre-existing data on block device, and this may lead to
data leakage. This patch add QCOW_OFLAG_ZERO for all preallocated
l2 entry to avoid this problem.
Signed-off-by: Ivan Ren
---
Changes in v2:
- always pass QCOW_OFLAG_ZERO when preallocate metadta
---
block/qcow2-cluster.c | 5 +++--
block/qcow2.c | 6 +++---
block/qcow2.h | 3 ++-
3 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 1aee726..b9e0ceb 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -919,7 +919,8 @@ fail:
return ret;
}
-int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m)
+int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m,
+uint64_t flags)
{
BDRVQcow2State *s = bs->opaque;
int i, j = 0, l2_index, ret;
@@ -969,7 +970,7 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs,
QCowL2Meta *m)
}
l2_slice[l2_index + i] = cpu_to_be64((cluster_offset +
-(i << s->cluster_bits)) | QCOW_OFLAG_COPIED);
+(i << s->cluster_bits)) | QCOW_OFLAG_COPIED | flags);
}
diff --git a/block/qcow2.c b/block/qcow2.c
index 2f36e63..a7aeea9 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2044,7 +2044,7 @@ static coroutine_fn int qcow2_co_pwritev(BlockDriverState
*bs, uint64_t offset,
while (l2meta != NULL) {
QCowL2Meta *next;
-ret = qcow2_alloc_cluster_link_l2(bs, l2meta);
+ret = qcow2_alloc_cluster_link_l2(bs, l2meta, 0);
if (ret < 0) {
goto fail;
}
@@ -2552,7 +2552,7 @@ static void coroutine_fn preallocate_co(void *opaque)
while (meta) {
QCowL2Meta *next = meta->next;
-ret = qcow2_alloc_cluster_link_l2(bs, meta);
+ret = qcow2_alloc_cluster_link_l2(bs, meta, QCOW_OFLAG_ZERO);
if (ret < 0) {
qcow2_free_any_clusters(bs, meta->alloc_offset,
meta->nb_clusters,
QCOW2_DISCARD_NEVER);
@@ -3458,7 +3458,7 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t
offset,
};
qemu_co_queue_init(&allocation.dependent_requests);
-ret = qcow2_alloc_cluster_link_l2(bs, &allocation);
+ret = qcow2_alloc_cluster_link_l2(bs, &allocation, 0);
if (ret < 0) {
error_setg_errno(errp, -ret, "Failed to update L2 tables");
qcow2_free_clusters(bs, host_offset,
diff --git a/block/qcow2.h b/block/qcow2.h
index adf5c39..9a59602 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -617,7 +617,8 @@ uint64_t
qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,
uint64_t offset,
int compressed_size);
-int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m);
+int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m,
+uint64_t flags);
int qcow2_cluster_discard(BlockDriverState *bs, uint64_t offset,
uint64_t bytes, enum qcow2_discard_type type,
bool full_discard);
--
1.8.3.1
[Qemu-block] [PATCH v4 10/10] qemu-img: Convert with copy offloading
The new blk_co_copy_range interface offers a more efficient way in the
case of network based storage. Make use of it to allow faster convert
operation.
Since copy offloading cannot do zero detection ('-S') and compression
(-c), only try it when these options are not used.
Signed-off-by: Fam Zheng
Reviewed-by: Stefan Hajnoczi
---
qemu-img.c | 50 --
1 file changed, 48 insertions(+), 2 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index ea62d2d61e..9c3678ebf0 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1544,6 +1544,7 @@ typedef struct ImgConvertState {
bool compressed;
bool target_has_backing;
bool wr_in_order;
+bool copy_range;
int min_sparse;
size_t cluster_sectors;
size_t buf_sectors;
@@ -1737,6 +1738,37 @@ static int coroutine_fn convert_co_write(ImgConvertState
*s, int64_t sector_num,
return 0;
}
+static int coroutine_fn convert_co_copy_range(ImgConvertState *s, int64_t
sector_num,
+ int nb_sectors)
+{
+int n, ret;
+
+while (nb_sectors > 0) {
+BlockBackend *blk;
+int src_cur;
+int64_t bs_sectors, src_cur_offset;
+int64_t offset;
+
+convert_select_part(s, sector_num, &src_cur, &src_cur_offset);
+offset = (sector_num - src_cur_offset) << BDRV_SECTOR_BITS;
+blk = s->src[src_cur];
+bs_sectors = s->src_sectors[src_cur];
+
+n = MIN(nb_sectors, bs_sectors - (sector_num - src_cur_offset));
+
+ret = blk_co_copy_range(blk, offset, s->target,
+sector_num << BDRV_SECTOR_BITS,
+n << BDRV_SECTOR_BITS, 0);
+if (ret < 0) {
+return ret;
+}
+
+sector_num += n;
+nb_sectors -= n;
+}
+return 0;
+}
+
static void coroutine_fn convert_co_do_copy(void *opaque)
{
ImgConvertState *s = opaque;
@@ -1759,6 +1791,7 @@ static void coroutine_fn convert_co_do_copy(void *opaque)
int n;
int64_t sector_num;
enum ImgConvertBlockStatus status;
+bool copy_range;
qemu_co_mutex_lock(&s->lock);
if (s->ret != -EINPROGRESS || s->sector_num >= s->total_sectors) {
@@ -1788,7 +1821,9 @@ static void coroutine_fn convert_co_do_copy(void *opaque)
s->allocated_sectors, 0);
}
-if (status == BLK_DATA) {
+retry:
+copy_range = s->copy_range && s->status == BLK_DATA;
+if (status == BLK_DATA && !copy_range) {
ret = convert_co_read(s, sector_num, n, buf);
if (ret < 0) {
error_report("error while reading sector %" PRId64
@@ -1810,7 +1845,15 @@ static void coroutine_fn convert_co_do_copy(void *opaque)
}
if (s->ret == -EINPROGRESS) {
-ret = convert_co_write(s, sector_num, n, buf, status);
+if (copy_range) {
+ret = convert_co_copy_range(s, sector_num, n);
+if (ret) {
+s->copy_range = false;
+goto retry;
+}
+} else {
+ret = convert_co_write(s, sector_num, n, buf, status);
+}
if (ret < 0) {
error_report("error while writing sector %" PRId64
": %s", sector_num, strerror(-ret));
@@ -1933,6 +1976,7 @@ static int img_convert(int argc, char **argv)
ImgConvertState s = (ImgConvertState) {
/* Need at least 4k of zeros for sparse detection */
.min_sparse = 8,
+.copy_range = true,
.buf_sectors= IO_BUF_SIZE / BDRV_SECTOR_SIZE,
.wr_in_order= true,
.num_coroutines = 8,
@@ -1973,6 +2017,7 @@ static int img_convert(int argc, char **argv)
break;
case 'c':
s.compressed = true;
+s.copy_range = false;
break;
case 'o':
if (!is_valid_option_list(optarg)) {
@@ -2014,6 +2059,7 @@ static int img_convert(int argc, char **argv)
}
s.min_sparse = sval / BDRV_SECTOR_SIZE;
+s.copy_range = false;
break;
}
case 'p':
--
2.14.3
[Qemu-block] [PATCH v4 08/10] iscsi: Implement copy offloading
Issue EXTENDED COPY (LID1) command to implement the copy_range API.
The parameter data construction code is modified from libiscsi's
iscsi-dd.c.
Signed-off-by: Fam Zheng
Reviewed-by: Stefan Hajnoczi
---
block/iscsi.c| 219 +++
include/scsi/constants.h | 3 +
2 files changed, 222 insertions(+)
diff --git a/block/iscsi.c b/block/iscsi.c
index 32e755c021..2e8b193479 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -2205,6 +2205,221 @@ static void coroutine_fn
iscsi_co_invalidate_cache(BlockDriverState *bs,
iscsi_allocmap_invalidate(iscsilun);
}
+static int coroutine_fn iscsi_co_copy_range_from(BlockDriverState *bs,
+ BdrvChild *src,
+ uint64_t src_offset,
+ BdrvChild *dst,
+ uint64_t dst_offset,
+ uint64_t bytes,
+ BdrvRequestFlags flags)
+{
+return bdrv_co_copy_range_to(src, src_offset, dst, dst_offset, bytes,
flags);
+}
+
+static struct scsi_task *iscsi_xcopy_task(int param_len)
+{
+struct scsi_task *task;
+
+task = g_new0(struct scsi_task, 1);
+
+task->cdb[0] = EXTENDED_COPY;
+task->cdb[10]= (param_len >> 24) & 0xFF;
+task->cdb[11]= (param_len >> 16) & 0xFF;
+task->cdb[12]= (param_len >> 8) & 0xFF;
+task->cdb[13]= param_len & 0xFF;
+task->cdb_size = 16;
+task->xfer_dir = SCSI_XFER_WRITE;
+task->expxferlen = param_len;
+
+return task;
+}
+
+static void iscsi_populate_target_desc(unsigned char *desc, IscsiLun *lun)
+{
+struct scsi_inquiry_device_designator *dd = lun->dd;
+
+memset(desc, 0, 32);
+desc[0] = IDENT_DESCR_TGT_DESCR;
+desc[4] = dd->code_set;
+desc[5] = (dd->designator_type & 0xF)
+| ((dd->association & 3) << 4);
+desc[7] = dd->designator_length;
+memcpy(desc + 8, dd->designator, dd->designator_length);
+
+desc[28] = 0;
+desc[29] = (lun->block_size >> 16) & 0xFF;
+desc[30] = (lun->block_size >> 8) & 0xFF;
+desc[31] = lun->block_size & 0xFF;
+}
+
+static void iscsi_xcopy_desc_hdr(uint8_t *hdr, int dc, int cat, int src_index,
+ int dst_index)
+{
+hdr[0] = 0x02; /* BLK_TO_BLK_SEG_DESCR */
+hdr[1] = ((dc << 1) | cat) & 0xFF;
+hdr[2] = (XCOPY_BLK2BLK_SEG_DESC_SIZE >> 8) & 0xFF;
+/* don't account for the first 4 bytes in descriptor header*/
+hdr[3] = (XCOPY_BLK2BLK_SEG_DESC_SIZE - 4 /* SEG_DESC_SRC_INDEX_OFFSET */)
& 0xFF;
+hdr[4] = (src_index >> 8) & 0xFF;
+hdr[5] = src_index & 0xFF;
+hdr[6] = (dst_index >> 8) & 0xFF;
+hdr[7] = dst_index & 0xFF;
+}
+
+static void iscsi_xcopy_populate_desc(uint8_t *desc, int dc, int cat,
+ int src_index, int dst_index, int
num_blks,
+ uint64_t src_lba, uint64_t dst_lba)
+{
+iscsi_xcopy_desc_hdr(desc, dc, cat, src_index, dst_index);
+
+/* The caller should verify the request size */
+assert(num_blks < 65536);
+desc[10] = (num_blks >> 8) & 0xFF;
+desc[11] = num_blks & 0xFF;
+desc[12] = (src_lba >> 56) & 0xFF;
+desc[13] = (src_lba >> 48) & 0xFF;
+desc[14] = (src_lba >> 40) & 0xFF;
+desc[15] = (src_lba >> 32) & 0xFF;
+desc[16] = (src_lba >> 24) & 0xFF;
+desc[17] = (src_lba >> 16) & 0xFF;
+desc[18] = (src_lba >> 8) & 0xFF;
+desc[19] = src_lba & 0xFF;
+desc[20] = (dst_lba >> 56) & 0xFF;
+desc[21] = (dst_lba >> 48) & 0xFF;
+desc[22] = (dst_lba >> 40) & 0xFF;
+desc[23] = (dst_lba >> 32) & 0xFF;
+desc[24] = (dst_lba >> 24) & 0xFF;
+desc[25] = (dst_lba >> 16) & 0xFF;
+desc[26] = (dst_lba >> 8) & 0xFF;
+desc[27] = dst_lba & 0xFF;
+}
+
+static void iscsi_xcopy_populate_header(unsigned char *buf, int list_id, int
str,
+int list_id_usage, int prio,
+int tgt_desc_len,
+int seg_desc_len, int inline_data_len)
+{
+buf[0] = list_id;
+buf[1] = ((str & 1) << 5) | ((list_id_usage & 3) << 3) | (prio & 7);
+buf[2] = (tgt_desc_len >> 8) & 0xFF;
+buf[3] = tgt_desc_len & 0xFF;
+buf[8] = (seg_desc_len >> 24) & 0xFF;
+buf[9] = (seg_desc_len >> 16) & 0xFF;
+buf[10] = (seg_desc_len >> 8) & 0xFF;
+buf[11] = seg_desc_len & 0xFF;
+buf[12] = (inline_data_len >> 24) & 0xFF;
+buf[13] = (inline_data_len >> 16) & 0xFF;
+buf[14] = (inline_data_len >> 8) & 0xFF;
+buf[15] = inline_data_len & 0xFF;
+}
+
+static void iscsi_xcopy_data(struct iscsi_data *data,
+ IscsiLun *src, int64_t src_lba,
+ IscsiLun *dst, int64_t dst_lba,
+ uint16_t num_blocks)
+{
+uint8_t *
[Qemu-block] [PATCH v4 09/10] block-backend: Add blk_co_copy_range
It's a BlockBackend wrapper of the BDS interface.
Signed-off-by: Fam Zheng
Reviewed-by: Stefan Hajnoczi
---
block/block-backend.c | 18 ++
include/sysemu/block-backend.h | 4
2 files changed, 22 insertions(+)
diff --git a/block/block-backend.c b/block/block-backend.c
index 681b240b12..5562ec4238 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2217,3 +2217,21 @@ void blk_unregister_buf(BlockBackend *blk, void *host)
{
bdrv_unregister_buf(blk_bs(blk), host);
}
+
+int coroutine_fn blk_co_copy_range(BlockBackend *blk_in, int64_t off_in,
+ BlockBackend *blk_out, int64_t off_out,
+ int bytes, BdrvRequestFlags flags)
+{
+int r;
+r = blk_check_byte_request(blk_in, off_in, bytes);
+if (r) {
+return r;
+}
+r = blk_check_byte_request(blk_out, off_out, bytes);
+if (r) {
+return r;
+}
+return bdrv_co_copy_range(blk_in->root, off_in,
+ blk_out->root, off_out,
+ bytes, flags);
+}
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 92ab624fac..6f043b6b51 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -232,4 +232,8 @@ void blk_set_force_allow_inactivate(BlockBackend *blk);
void blk_register_buf(BlockBackend *blk, void *host, size_t size);
void blk_unregister_buf(BlockBackend *blk, void *host);
+int blk_co_copy_range(BlockBackend *blk_in, int64_t off_in,
+ BlockBackend *blk_out, int64_t off_out,
+ int bytes, BdrvRequestFlags flags);
+
#endif
--
2.14.3
[Qemu-block] [PATCH v4 07/10] iscsi: Create and use iscsi_co_wait_for_task
This loop is repeated a growing number times. Make a helper.
Signed-off-by: Fam Zheng
Reviewed-by: Stefan Hajnoczi
Reviewed-by: Eric Blake
---
block/iscsi.c | 54 +-
1 file changed, 17 insertions(+), 37 deletions(-)
diff --git a/block/iscsi.c b/block/iscsi.c
index 5811ff1d51..32e755c021 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -556,6 +556,17 @@ static inline bool iscsi_allocmap_is_valid(IscsiLun
*iscsilun,
offset / iscsilun->cluster_size) == size);
}
+static void coroutine_fn iscsi_co_wait_for_task(IscsiTask *iTask,
+IscsiLun *iscsilun)
+{
+while (!iTask->complete) {
+iscsi_set_events(iscsilun);
+qemu_mutex_unlock(&iscsilun->mutex);
+qemu_coroutine_yield();
+qemu_mutex_lock(&iscsilun->mutex);
+}
+}
+
static int coroutine_fn
iscsi_co_writev_flags(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
QEMUIOVector *iov, int flags)
@@ -617,12 +628,7 @@ retry:
scsi_task_set_iov_out(iTask.task, (struct scsi_iovec *) iov->iov,
iov->niov);
#endif
-while (!iTask.complete) {
-iscsi_set_events(iscsilun);
-qemu_mutex_unlock(&iscsilun->mutex);
-qemu_coroutine_yield();
-qemu_mutex_lock(&iscsilun->mutex);
-}
+iscsi_co_wait_for_task(&iTask, iscsilun);
if (iTask.task != NULL) {
scsi_free_scsi_task(iTask.task);
@@ -693,13 +699,7 @@ retry:
ret = -ENOMEM;
goto out_unlock;
}
-
-while (!iTask.complete) {
-iscsi_set_events(iscsilun);
-qemu_mutex_unlock(&iscsilun->mutex);
-qemu_coroutine_yield();
-qemu_mutex_lock(&iscsilun->mutex);
-}
+iscsi_co_wait_for_task(&iTask, iscsilun);
if (iTask.do_retry) {
if (iTask.task != NULL) {
@@ -863,13 +863,8 @@ retry:
#if LIBISCSI_API_VERSION < (20160603)
scsi_task_set_iov_in(iTask.task, (struct scsi_iovec *) iov->iov,
iov->niov);
#endif
-while (!iTask.complete) {
-iscsi_set_events(iscsilun);
-qemu_mutex_unlock(&iscsilun->mutex);
-qemu_coroutine_yield();
-qemu_mutex_lock(&iscsilun->mutex);
-}
+iscsi_co_wait_for_task(&iTask, iscsilun);
if (iTask.task != NULL) {
scsi_free_scsi_task(iTask.task);
iTask.task = NULL;
@@ -906,12 +901,7 @@ retry:
return -ENOMEM;
}
-while (!iTask.complete) {
-iscsi_set_events(iscsilun);
-qemu_mutex_unlock(&iscsilun->mutex);
-qemu_coroutine_yield();
-qemu_mutex_lock(&iscsilun->mutex);
-}
+iscsi_co_wait_for_task(&iTask, iscsilun);
if (iTask.task != NULL) {
scsi_free_scsi_task(iTask.task);
@@ -1143,12 +1133,7 @@ retry:
goto out_unlock;
}
-while (!iTask.complete) {
-iscsi_set_events(iscsilun);
-qemu_mutex_unlock(&iscsilun->mutex);
-qemu_coroutine_yield();
-qemu_mutex_lock(&iscsilun->mutex);
-}
+iscsi_co_wait_for_task(&iTask, iscsilun);
if (iTask.task != NULL) {
scsi_free_scsi_task(iTask.task);
@@ -1244,12 +1229,7 @@ retry:
return -ENOMEM;
}
-while (!iTask.complete) {
-iscsi_set_events(iscsilun);
-qemu_mutex_unlock(&iscsilun->mutex);
-qemu_coroutine_yield();
-qemu_mutex_lock(&iscsilun->mutex);
-}
+iscsi_co_wait_for_task(&iTask, iscsilun);
if (iTask.status == SCSI_STATUS_CHECK_CONDITION &&
iTask.task->sense.key == SCSI_SENSE_ILLEGAL_REQUEST &&
--
2.14.3
[Qemu-block] [PATCH v4 06/10] iscsi: Query and save device designator when opening
The device designator data returned in INQUIRY command will be useful to
fill in source/target fields during copy offloading. Do this when
connecting to the target and save the data for later use.
Signed-off-by: Fam Zheng
Reviewed-by: Stefan Hajnoczi
---
block/iscsi.c| 41 +
include/scsi/constants.h | 2 ++
2 files changed, 43 insertions(+)
diff --git a/block/iscsi.c b/block/iscsi.c
index d19ae0e398..5811ff1d51 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -68,6 +68,7 @@ typedef struct IscsiLun {
QemuMutex mutex;
struct scsi_inquiry_logical_block_provisioning lbp;
struct scsi_inquiry_block_limits bl;
+struct scsi_inquiry_device_designator *dd;
unsigned char *zeroblock;
/* The allocmap tracks which clusters (pages) on the iSCSI target are
* allocated and which are not. In case a target returns zeros for
@@ -1740,6 +1741,30 @@ static QemuOptsList runtime_opts = {
},
};
+static void iscsi_save_designator(IscsiLun *lun,
+ struct scsi_inquiry_device_identification
*inq_di)
+{
+struct scsi_inquiry_device_designator *desig, *copy = NULL;
+
+for (desig = inq_di->designators; desig; desig = desig->next) {
+if (desig->association ||
+desig->designator_type > SCSI_DESIGNATOR_TYPE_NAA) {
+continue;
+}
+/* NAA works better than T10 vendor ID based designator. */
+if (!copy || copy->designator_type < desig->designator_type) {
+copy = desig;
+}
+}
+if (copy) {
+lun->dd = g_new(struct scsi_inquiry_device_designator, 1);
+*lun->dd = *copy;
+lun->dd->next = NULL;
+lun->dd->designator = g_malloc(copy->designator_length);
+memcpy(lun->dd->designator, copy->designator, copy->designator_length);
+}
+}
+
static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
@@ -1922,6 +1947,7 @@ static int iscsi_open(BlockDriverState *bs, QDict
*options, int flags,
struct scsi_task *inq_task;
struct scsi_inquiry_logical_block_provisioning *inq_lbp;
struct scsi_inquiry_block_limits *inq_bl;
+struct scsi_inquiry_device_identification *inq_di;
switch (inq_vpd->pages[i]) {
case SCSI_INQUIRY_PAGECODE_LOGICAL_BLOCK_PROVISIONING:
inq_task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 1,
@@ -1947,6 +1973,17 @@ static int iscsi_open(BlockDriverState *bs, QDict
*options, int flags,
sizeof(struct scsi_inquiry_block_limits));
scsi_free_scsi_task(inq_task);
break;
+case SCSI_INQUIRY_PAGECODE_DEVICE_IDENTIFICATION:
+inq_task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 1,
+
SCSI_INQUIRY_PAGECODE_DEVICE_IDENTIFICATION,
+(void **) &inq_di, errp);
+if (inq_task == NULL) {
+ret = -EINVAL;
+goto out;
+}
+iscsi_save_designator(iscsilun, inq_di);
+scsi_free_scsi_task(inq_task);
+break;
default:
break;
}
@@ -2003,6 +2040,10 @@ static void iscsi_close(BlockDriverState *bs)
iscsi_logout_sync(iscsi);
}
iscsi_destroy_context(iscsi);
+if (iscsilun->dd) {
+g_free(iscsilun->dd->designator);
+g_free(iscsilun->dd);
+}
g_free(iscsilun->zeroblock);
iscsi_allocmap_free(iscsilun);
qemu_mutex_destroy(&iscsilun->mutex);
diff --git a/include/scsi/constants.h b/include/scsi/constants.h
index a141dd71f8..54733b7110 100644
--- a/include/scsi/constants.h
+++ b/include/scsi/constants.h
@@ -311,4 +311,6 @@
#define MMC_PROFILE_HDDVD_RW_DL 0x005A
#define MMC_PROFILE_INVALID 0x
+#define IDENT_DESCR_TGT_DESCR 0xE4
+
#endif
--
2.14.3
[Qemu-block] [PATCH v4 04/10] qcow2: Implement copy offloading
The two callbacks are implemented quite similarly to the read/write
functions: bdrv_co_copy_range_from maps for read and calls into bs->file
or bs->backing depending on the allocation status; bdrv_co_copy_range_to
maps for write and calls into bs->file.
Signed-off-by: Fam Zheng
---
block/qcow2.c | 232 ++
1 file changed, 202 insertions(+), 30 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 2f36e632f9..b5a8b2c1f7 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1755,6 +1755,31 @@ static int coroutine_fn
qcow2_co_block_status(BlockDriverState *bs,
return status;
}
+static int qcow2_handle_l2meta(BlockDriverState *bs, QCowL2Meta *l2meta)
+{
+int ret = 0;
+
+while (l2meta != NULL) {
+QCowL2Meta *next;
+
+if (!ret) {
+ret = qcow2_alloc_cluster_link_l2(bs, l2meta);
+}
+
+/* Take the request off the list of running requests */
+if (l2meta->nb_clusters != 0) {
+QLIST_REMOVE(l2meta, next_in_flight);
+}
+
+qemu_co_queue_restart_all(&l2meta->dependent_requests);
+
+next = l2meta->next;
+g_free(l2meta);
+l2meta = next;
+}
+return ret;
+}
+
static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
uint64_t bytes, QEMUIOVector *qiov,
int flags)
@@ -2041,24 +2066,10 @@ static coroutine_fn int
qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset,
}
}
-while (l2meta != NULL) {
-QCowL2Meta *next;
-
-ret = qcow2_alloc_cluster_link_l2(bs, l2meta);
-if (ret < 0) {
-goto fail;
-}
-
-/* Take the request off the list of running requests */
-if (l2meta->nb_clusters != 0) {
-QLIST_REMOVE(l2meta, next_in_flight);
-}
-
-qemu_co_queue_restart_all(&l2meta->dependent_requests);
-
-next = l2meta->next;
-g_free(l2meta);
-l2meta = next;
+ret = qcow2_handle_l2meta(bs, l2meta);
+l2meta = NULL;
+if (ret) {
+goto fail;
}
bytes -= cur_bytes;
@@ -2069,18 +2080,7 @@ static coroutine_fn int
qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset,
ret = 0;
fail:
-while (l2meta != NULL) {
-QCowL2Meta *next;
-
-if (l2meta->nb_clusters != 0) {
-QLIST_REMOVE(l2meta, next_in_flight);
-}
-qemu_co_queue_restart_all(&l2meta->dependent_requests);
-
-next = l2meta->next;
-g_free(l2meta);
-l2meta = next;
-}
+qcow2_handle_l2meta(bs, l2meta);
qemu_co_mutex_unlock(&s->lock);
@@ -3267,6 +3267,176 @@ static coroutine_fn int
qcow2_co_pdiscard(BlockDriverState *bs,
return ret;
}
+static int qcow2_co_copy_range_from(BlockDriverState *bs,
+BdrvChild *src, uint64_t src_offset,
+BdrvChild *dst, uint64_t dst_offset,
+uint64_t bytes, BdrvRequestFlags flags)
+{
+BDRVQcow2State *s = bs->opaque;
+int ret;
+unsigned int cur_bytes; /* number of bytes in current iteration */
+BdrvChild *child = NULL;
+
+assert(!bs->encrypted);
+qemu_co_mutex_lock(&s->lock);
+
+while (bytes != 0) {
+uint64_t copy_offset = 0;
+/* prepare next request */
+cur_bytes = MIN(bytes, INT_MAX);
+
+ret = qcow2_get_cluster_offset(bs, src_offset, &cur_bytes,
©_offset);
+if (ret < 0) {
+goto out;
+}
+
+switch (ret) {
+case QCOW2_CLUSTER_UNALLOCATED:
+if (bs->backing && bs->backing->bs) {
+int64_t backing_length = bdrv_getlength(bs->backing->bs);
+if (src_offset >= backing_length) {
+flags |= BDRV_REQ_ZERO_WRITE;
+} else {
+child = bs->backing;
+cur_bytes = MIN(cur_bytes, backing_length - src_offset);
+copy_offset = src_offset;
+}
+} else {
+flags |= BDRV_REQ_ZERO_WRITE;
+}
+break;
+
+case QCOW2_CLUSTER_ZERO_PLAIN:
+case QCOW2_CLUSTER_ZERO_ALLOC:
+flags |= BDRV_REQ_ZERO_WRITE;
+break;
+
+case QCOW2_CLUSTER_COMPRESSED:
+ret = -ENOTSUP;
+goto out;
+break;
+
+case QCOW2_CLUSTER_NORMAL:
+child = bs->file;
+copy_offset += offset_into_cluster(s, src_offset);
+if ((copy_offset & 511) != 0) {
+ret = -EIO;
+goto out;
+}
+break;
+
+default:
+abort();
+}
+qemu_co_mutex_unlock(&s->lock);
+ret = bdrv_co_copy_range_from(child,
+
[Qemu-block] [PATCH v4 03/10] raw: Implement copy offloading
Just pass down to ->file.
Signed-off-by: Fam Zheng
Reviewed-by: Stefan Hajnoczi
---
block/raw-format.c | 32
1 file changed, 32 insertions(+)
diff --git a/block/raw-format.c b/block/raw-format.c
index 803083f1a1..71ca9c627d 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -495,6 +495,36 @@ static int raw_probe_geometry(BlockDriverState *bs,
HDGeometry *geo)
return bdrv_probe_geometry(bs->file->bs, geo);
}
+static int coroutine_fn raw_co_copy_range_from(BlockDriverState *bs,
+ BdrvChild *src, uint64_t
src_offset,
+ BdrvChild *dst, uint64_t
dst_offset,
+ uint64_t bytes,
BdrvRequestFlags flags)
+{
+int ret;
+
+ret = raw_adjust_offset(bs, &src_offset, bytes);
+if (ret) {
+return ret;
+}
+return bdrv_co_copy_range_from(bs->file, src_offset, dst, dst_offset,
+ bytes, flags);
+}
+
+static int coroutine_fn raw_co_copy_range_to(BlockDriverState *bs,
+ BdrvChild *src, uint64_t
src_offset,
+ BdrvChild *dst, uint64_t
dst_offset,
+ uint64_t bytes, BdrvRequestFlags
flags)
+{
+int ret;
+
+ret = raw_adjust_offset(bs, &dst_offset, bytes);
+if (ret) {
+return ret;
+}
+return bdrv_co_copy_range_to(src, src_offset, bs->file, dst_offset, bytes,
+ flags);
+}
+
BlockDriver bdrv_raw = {
.format_name = "raw",
.instance_size= sizeof(BDRVRawState),
@@ -511,6 +541,8 @@ BlockDriver bdrv_raw = {
.bdrv_co_pwrite_zeroes = &raw_co_pwrite_zeroes,
.bdrv_co_pdiscard = &raw_co_pdiscard,
.bdrv_co_block_status = &raw_co_block_status,
+.bdrv_co_copy_range_from = &raw_co_copy_range_from,
+.bdrv_co_copy_range_to = &raw_co_copy_range_to,
.bdrv_truncate= &raw_truncate,
.bdrv_getlength = &raw_getlength,
.has_variable_length = true,
--
2.14.3
[Qemu-block] [PATCH v4 05/10] file-posix: Implement bdrv_co_copy_range
With copy_file_range(2), we can implement the bdrv_co_copy_range
semantics.
Signed-off-by: Fam Zheng
---
block/file-posix.c | 96 +++--
include/block/raw-aio.h | 10 --
2 files changed, 101 insertions(+), 5 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index 3794c0007a..7b5319fa06 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -59,6 +59,7 @@
#ifdef __linux__
#include
#include
+#include
#include
#include
#include
@@ -185,6 +186,8 @@ typedef struct RawPosixAIOData {
#define aio_ioctl_cmd aio_nbytes /* for QEMU_AIO_IOCTL */
off_t aio_offset;
int aio_type;
+int aio_fd2;
+off_t aio_offset2;
} RawPosixAIOData;
#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
@@ -1421,6 +1424,47 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData
*aiocb)
return -ENOTSUP;
}
+static off_t copy_file_range(int in_fd, off_t *in_off, int out_fd,
+ off_t *out_off, size_t len, unsigned int flags)
+{
+#ifdef __NR_copy_file_range
+return syscall(__NR_copy_file_range, in_fd, in_off, out_fd,
+ out_off, len, flags);
+#else
+errno = ENOSYS;
+return -1;
+#endif
+}
+
+static ssize_t handle_aiocb_copy_range(RawPosixAIOData *aiocb)
+{
+uint64_t bytes = aiocb->aio_nbytes;
+off_t in_off = aiocb->aio_offset;
+off_t out_off = aiocb->aio_offset2;
+
+while (bytes) {
+ssize_t ret = copy_file_range(aiocb->aio_fildes, &in_off,
+ aiocb->aio_fd2, &out_off,
+ bytes, 0);
+if (ret == -EINTR) {
+continue;
+}
+if (ret < 0) {
+if (errno == ENOSYS) {
+return -ENOTSUP;
+} else {
+return -errno;
+}
+}
+if (!ret) {
+/* No progress (e.g. when beyond EOF), fall back to buffer I/O. */
+return -ENOTSUP;
+}
+bytes -= ret;
+}
+return 0;
+}
+
static ssize_t handle_aiocb_discard(RawPosixAIOData *aiocb)
{
int ret = -EOPNOTSUPP;
@@ -1501,6 +1545,9 @@ static int aio_worker(void *arg)
case QEMU_AIO_WRITE_ZEROES:
ret = handle_aiocb_write_zeroes(aiocb);
break;
+case QEMU_AIO_COPY_RANGE:
+ret = handle_aiocb_copy_range(aiocb);
+break;
default:
fprintf(stderr, "invalid aio request (0x%x)\n", aiocb->aio_type);
ret = -EINVAL;
@@ -1511,9 +1558,10 @@ static int aio_worker(void *arg)
return ret;
}
-static int paio_submit_co(BlockDriverState *bs, int fd,
- int64_t offset, QEMUIOVector *qiov,
- int bytes, int type)
+static int paio_submit_co_full(BlockDriverState *bs, int fd,
+ int64_t offset, int fd2, int64_t offset2,
+ QEMUIOVector *qiov,
+ int bytes, int type)
{
RawPosixAIOData *acb = g_new(RawPosixAIOData, 1);
ThreadPool *pool;
@@ -1521,6 +1569,8 @@ static int paio_submit_co(BlockDriverState *bs, int fd,
acb->bs = bs;
acb->aio_type = type;
acb->aio_fildes = fd;
+acb->aio_fd2 = fd2;
+acb->aio_offset2 = offset2;
acb->aio_nbytes = bytes;
acb->aio_offset = offset;
@@ -1536,6 +1586,13 @@ static int paio_submit_co(BlockDriverState *bs, int fd,
return thread_pool_submit_co(pool, aio_worker, acb);
}
+static inline int paio_submit_co(BlockDriverState *bs, int fd,
+ int64_t offset, QEMUIOVector *qiov,
+ int bytes, int type)
+{
+return paio_submit_co_full(bs, fd, offset, -1, 0, qiov, bytes, type);
+}
+
static BlockAIOCB *paio_submit(BlockDriverState *bs, int fd,
int64_t offset, QEMUIOVector *qiov, int bytes,
BlockCompletionFunc *cb, void *opaque, int type)
@@ -2312,6 +2369,35 @@ static void raw_abort_perm_update(BlockDriverState *bs)
raw_handle_perm_lock(bs, RAW_PL_ABORT, 0, 0, NULL);
}
+static int coroutine_fn raw_co_copy_range_from(BlockDriverState *bs,
+ BdrvChild *src, uint64_t src_offset,
+ BdrvChild *dst, uint64_t dst_offset,
+ uint64_t bytes, BdrvRequestFlags flags)
+{
+return bdrv_co_copy_range_to(src, src_offset, dst, dst_offset, bytes,
flags);
+}
+
+static int coroutine_fn raw_co_copy_range_to(BlockDriverState *bs,
+ BdrvChild *src, uint64_t
src_offset,
+ BdrvChild *dst, uint64_t
dst_offset,
+ uint64_t bytes, BdrvRequestFlags
flags)
+{
+BDRVRawState *s = bs->opaque;
+BDRVRawState *src_s;
+
+assert(dst->bs == bs);
+if (src->bs->drv->bdrv_co_copy_range_to != raw_co_copy_range_to) {
+return -ENOTSUP;
+
[Qemu-block] [PATCH v4 02/10] raw: Check byte range uniformly
We don't verify the request range against s->size in the I/O callbacks
except for raw_co_pwritev. This is wrong (especially for
raw_co_pwrite_zeroes and raw_co_pdiscard), so fix them.
Signed-off-by: Fam Zheng
---
block/raw-format.c | 63 --
1 file changed, 38 insertions(+), 25 deletions(-)
diff --git a/block/raw-format.c b/block/raw-format.c
index a378547c99..803083f1a1 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -167,16 +167,36 @@ static void raw_reopen_abort(BDRVReopenState *state)
state->opaque = NULL;
}
+/* Check and adjust the offset, against 'offset' and 'size' options. */
+static inline int raw_adjust_offset(BlockDriverState *bs, uint64_t *offset,
+uint64_t bytes)
+{
+BDRVRawState *s = bs->opaque;
+
+if (s->has_size && (*offset > s->size || bytes > (s->size - *offset))) {
+/* There's not enough space for the data. Don't write anything and just
+ * fail to prevent leaking out of the size specified in options. */
+return -ENOSPC;
+}
+
+if (*offset > UINT64_MAX - s->offset) {
+return -EINVAL;
+}
+*offset += s->offset;
+
+return 0;
+}
+
static int coroutine_fn raw_co_preadv(BlockDriverState *bs, uint64_t offset,
uint64_t bytes, QEMUIOVector *qiov,
int flags)
{
-BDRVRawState *s = bs->opaque;
+int ret;
-if (offset > UINT64_MAX - s->offset) {
-return -EINVAL;
+ret = raw_adjust_offset(bs, &offset, bytes);
+if (ret) {
+return ret;
}
-offset += s->offset;
BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO);
return bdrv_co_preadv(bs->file, offset, bytes, qiov, flags);
@@ -186,23 +206,11 @@ static int coroutine_fn raw_co_pwritev(BlockDriverState
*bs, uint64_t offset,
uint64_t bytes, QEMUIOVector *qiov,
int flags)
{
-BDRVRawState *s = bs->opaque;
void *buf = NULL;
BlockDriver *drv;
QEMUIOVector local_qiov;
int ret;
-if (s->has_size && (offset > s->size || bytes > (s->size - offset))) {
-/* There's not enough space for the data. Don't write anything and just
- * fail to prevent leaking out of the size specified in options. */
-return -ENOSPC;
-}
-
-if (offset > UINT64_MAX - s->offset) {
-ret = -EINVAL;
-goto fail;
-}
-
if (bs->probed && offset < BLOCK_PROBE_BUF_SIZE && bytes) {
/* Handling partial writes would be a pain - so we just
* require that guests have 512-byte request alignment if
@@ -237,7 +245,10 @@ static int coroutine_fn raw_co_pwritev(BlockDriverState
*bs, uint64_t offset,
qiov = &local_qiov;
}
-offset += s->offset;
+ret = raw_adjust_offset(bs, &offset, bytes);
+if (ret) {
+goto fail;
+}
BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO);
ret = bdrv_co_pwritev(bs->file, offset, bytes, qiov, flags);
@@ -267,22 +278,24 @@ static int coroutine_fn
raw_co_pwrite_zeroes(BlockDriverState *bs,
int64_t offset, int bytes,
BdrvRequestFlags flags)
{
-BDRVRawState *s = bs->opaque;
-if (offset > UINT64_MAX - s->offset) {
-return -EINVAL;
+int ret;
+
+ret = raw_adjust_offset(bs, (uint64_t *)&offset, bytes);
+if (ret) {
+return ret;
}
-offset += s->offset;
return bdrv_co_pwrite_zeroes(bs->file, offset, bytes, flags);
}
static int coroutine_fn raw_co_pdiscard(BlockDriverState *bs,
int64_t offset, int bytes)
{
-BDRVRawState *s = bs->opaque;
-if (offset > UINT64_MAX - s->offset) {
-return -EINVAL;
+int ret;
+
+ret = raw_adjust_offset(bs, (uint64_t *)&offset, bytes);
+if (ret) {
+return ret;
}
-offset += s->offset;
return bdrv_co_pdiscard(bs->file->bs, offset, bytes);
}
--
2.14.3
[Qemu-block] [PATCH v4 00/10] qemu-img convert with copy offloading
v4: - Fix raw offset and size. [Eric] - iscsi: Drop unnecessary return values and variables in favor of constants. [Stefan] - qcow2: Handle small backing case. [Stefan] - file-posix: Translate ENOSYS to ENOTSUP. [Stefan] - API documentation and commit message. [Stefan] - Add rev-by to patches 3, 5 - 10. [Stefan, Eric] This series introduces block layer API for copy offloading and makes use of it in qemu-img convert. For now we implemented the operation in local file protocol with copy_file_range(2). Besides that it's possible to add similar to iscsi, nfs and potentially more. As far as its usage goes, in addition to qemu-img convert, we can emulate offloading in scsi-disk (handle EXTENDED COPY command), and use the API in block jobs too. Fam Zheng (10): block: Introduce API for copy offloading raw: Check byte range uniformly raw: Implement copy offloading qcow2: Implement copy offloading file-posix: Implement bdrv_co_copy_range iscsi: Query and save device designator when opening iscsi: Create and use iscsi_co_wait_for_task iscsi: Implement copy offloading block-backend: Add blk_co_copy_range qemu-img: Convert with copy offloading block/block-backend.c | 18 +++ block/file-posix.c | 96 - block/io.c | 96 + block/iscsi.c | 314 - block/qcow2.c | 232 ++ block/raw-format.c | 95 + include/block/block.h | 32 + include/block/block_int.h | 38 + include/block/raw-aio.h| 10 +- include/scsi/constants.h | 5 + include/sysemu/block-backend.h | 4 + qemu-img.c | 50 ++- 12 files changed, 891 insertions(+), 99 deletions(-) -- 2.14.3
[Qemu-block] [PATCH v4 01/10] block: Introduce API for copy offloading
Introduce the bdrv_co_copy_range() API for copy offloading. Block
drivers implementing this API support efficient copy operations that
avoid reading each block from the source device and writing it to the
destination devices. Examples of copy offload primitives are SCSI
EXTENDED COPY and Linux copy_file_range(2).
Signed-off-by: Fam Zheng
---
block/io.c| 96 +++
include/block/block.h | 32
include/block/block_int.h | 38 +++
3 files changed, 166 insertions(+)
diff --git a/block/io.c b/block/io.c
index bd9a19a9c4..79e0195bc6 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2826,3 +2826,99 @@ void bdrv_unregister_buf(BlockDriverState *bs, void
*host)
bdrv_unregister_buf(child->bs, host);
}
}
+
+static int bdrv_co_copy_range_internal(BdrvChild *src,
+ uint64_t src_offset,
+ BdrvChild *dst,
+ uint64_t dst_offset,
+ uint64_t bytes, BdrvRequestFlags flags,
+ bool recurse_src)
+{
+int ret;
+
+if (!src || !dst || !src->bs || !dst->bs) {
+return -ENOMEDIUM;
+}
+ret = bdrv_check_byte_request(src->bs, src_offset, bytes);
+if (ret) {
+return ret;
+}
+
+ret = bdrv_check_byte_request(dst->bs, dst_offset, bytes);
+if (ret) {
+return ret;
+}
+if (flags & BDRV_REQ_ZERO_WRITE) {
+return bdrv_co_pwrite_zeroes(dst, dst_offset, bytes, flags);
+}
+
+if (!src->bs->drv->bdrv_co_copy_range_from
+|| !dst->bs->drv->bdrv_co_copy_range_to
+|| src->bs->encrypted || dst->bs->encrypted) {
+return -ENOTSUP;
+}
+if (recurse_src) {
+return src->bs->drv->bdrv_co_copy_range_from(src->bs,
+ src, src_offset,
+ dst, dst_offset,
+ bytes, flags);
+} else {
+return dst->bs->drv->bdrv_co_copy_range_to(dst->bs,
+ src, src_offset,
+ dst, dst_offset,
+ bytes, flags);
+}
+}
+
+/* Copy range from @src to @dst.
+ *
+ * See the comment of bdrv_co_copy_range for the parameter and return value
+ * semantics. */
+int bdrv_co_copy_range_from(BdrvChild *src, uint64_t src_offset,
+BdrvChild *dst, uint64_t dst_offset,
+uint64_t bytes, BdrvRequestFlags flags)
+{
+return bdrv_co_copy_range_internal(src, src_offset, dst, dst_offset,
+ bytes, flags, true);
+}
+
+/* Copy range from @src to @dst.
+ *
+ * See the comment of bdrv_co_copy_range for the parameter and return value
+ * semantics. */
+int bdrv_co_copy_range_to(BdrvChild *src, uint64_t src_offset,
+ BdrvChild *dst, uint64_t dst_offset,
+ uint64_t bytes, BdrvRequestFlags flags)
+{
+return bdrv_co_copy_range_internal(src, src_offset, dst, dst_offset,
+ bytes, flags, false);
+}
+
+int bdrv_co_copy_range(BdrvChild *src, uint64_t src_offset,
+ BdrvChild *dst, uint64_t dst_offset,
+ uint64_t bytes, BdrvRequestFlags flags)
+{
+BdrvTrackedRequest src_req, dst_req;
+BlockDriverState *src_bs = src->bs;
+BlockDriverState *dst_bs = dst->bs;
+int ret;
+
+bdrv_inc_in_flight(src_bs);
+bdrv_inc_in_flight(dst_bs);
+tracked_request_begin(&src_req, src_bs, src_offset,
+ bytes, BDRV_TRACKED_READ);
+tracked_request_begin(&dst_req, dst_bs, dst_offset,
+ bytes, BDRV_TRACKED_WRITE);
+
+wait_serialising_requests(&src_req);
+wait_serialising_requests(&dst_req);
+ret = bdrv_co_copy_range_from(src, src_offset,
+ dst, dst_offset,
+ bytes, flags);
+
+tracked_request_end(&src_req);
+tracked_request_end(&dst_req);
+bdrv_dec_in_flight(src_bs);
+bdrv_dec_in_flight(dst_bs);
+return ret;
+}
diff --git a/include/block/block.h b/include/block/block.h
index cdec3639a3..aa3a5326a8 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -604,4 +604,36 @@ bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs,
const char *name,
*/
void bdrv_register_buf(BlockDriverState *bs, void *host, size_t size);
void bdrv_unregister_buf(BlockDriverState *bs, void *host);
+
+/**
+ *
+ * bdrv_co_copy_range:
+ *
+ * Do offloaded copy between two children. If the operation is not implemented
+ * by the driver, or if the backend storage doesn't support it, a negative
+ * error code will be returned.
+ *
+ * Note:
Re: [Qemu-block] [Qemu-devel] [PATCH] qcow2: fix preallocation with metadata on bare block device
> This says what the patch does, but not why. What is the actual use case
> scenario where changing semantics to have the qcow2 overwrite the
> garbage to read 0 instead of any pre-existing garbage, when dealing with
> portions of the disk that have not yet been written by the guest? Are
> you trying to prevent a security leak of previous information that may
> be resident on the block device?
Yes, any pre-existing data on the device should not be read, otherwise it
may lead security leak in cloud environment.
For metadata preallocated qcow2 on bare block device, I think
QCOW_OFLAG_ZERO is more suitable to prevent this problem than other(such as
do "dd" before create a preallocated qcow2).
> > +/* Check the underlying device type.
> > + * If the underlying device is a block device, we add
> > + * QCOW_OFLAG_ZERO for all preallocated l2 entry to ignore
dirty
> > + * data on block device.
> > + * If the underlying device can't be used with stat(return
< 0),
> > + * treat it as a regular file.
> > + */
> > +if (stat(bs->filename, &st) < 0 || !S_ISBLK(st.st_mode)) {
>
> Won't work. You cannot guarantee that bs->filename is a local file; it
> could be a remote protocol, such as NBD or gluster. If you need to know
> whether bs->filename has a property that it reads as all zeroes when
> first initialized, we already have
> BlockDriverInfo::unallocated_blocks_are_zero, which is set to true for
> regular files and false for block devices.
Yes, BlockDriverInfo::unallocated_blocks_are_zero is enough and better to
distinguish the device.
> > +ret = qcow2_alloc_cluster_link_l2(bs, meta, 0);
> > +} else {
> > +ret = qcow2_alloc_cluster_link_l2(bs, meta,
QCOW_OFLAG_ZERO);
>
> Why would we not want to pass QCOW_OFLAG_ZERO always, rather than
> special-casing it based on what the underlying protocol might already
> have in place?
Yea, it sounds good. Always pass QCOW_OFLAG_ZERO in preallocation has no
problem and can guarantee no garbage will be read when preallcate metadata
for qcow2 on any underlying device.
I will send a v2 patch.
Thanks.
On Thu, May 10, 2018 at 11:57 PM Eric Blake wrote:
> On 05/08/2018 07:27 AM, Ivan Ren wrote:
> > Create a qcow2 directly on bare block device with
> > "-o preallocation=metadata" option. When read this qcow2, it will
> > return dirty data of block device.
>
> Yes, reading garbage is expected.
>
> > This patch add QCOW_OFLAG_ZERO
> > for all preallocated l2 entry if the underlying device is a bare
> > block device.
>
> This says what the patch does, but not why. What is the actual use case
> scenario where changing semantics to have the qcow2 overwrite the
> garbage to read 0 instead of any pre-existing garbage, when dealing with
> portions of the disk that have not yet been written by the guest? Are
> you trying to prevent a security leak of previous information that may
> be resident on the block device?
>
> >
> > Signed-off-by: Ivan Ren
> > ---
> > block/qcow2-cluster.c | 5 +++--
> > block/qcow2.c | 19 ---
> > block/qcow2.h | 3 ++-
> > 3 files changed, 21 insertions(+), 6 deletions(-)
> >
> > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> > index 1aee726..b9e0ceb 100644
> > --- a/block/qcow2-cluster.c
> > +++ b/block/qcow2-cluster.c
> > @@ -919,7 +919,8 @@ fail:
> > return ret;
> > }
> >
> > -int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m)
> > +int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m,
> > +uint64_t flags)
> > {
> > BDRVQcow2State *s = bs->opaque;
> > int i, j = 0, l2_index, ret;
> > @@ -969,7 +970,7 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState
> *bs, QCowL2Meta *m)
> > }
> >
> > l2_slice[l2_index + i] = cpu_to_be64((cluster_offset +
> > -(i << s->cluster_bits)) | QCOW_OFLAG_COPIED);
> > +(i << s->cluster_bits)) | QCOW_OFLAG_COPIED |
> flags);
> >}
> >
> >
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index 2f36e63..093735c 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -2044,7 +2044,7 @@ static coroutine_fn int
> qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset,
> > while (l2meta != NULL) {
> > QCowL2Meta *next;
> >
> > -ret = qcow2_alloc_cluster_link_l2(bs, l2meta);
> > +ret = qcow2_alloc_cluster_link_l2(bs, l2meta, 0);
> > if (ret < 0) {
> > goto fail;
> > }
> > @@ -2534,6 +2534,7 @@ static void coroutine_fn preallocate_co(void
> *opaque)
> > uint64_t host_offset = 0;
> > unsigned int cur_bytes;
> > int ret;
> > +struct stat st;
> > QCowL2Meta *meta;
> >
> > qemu_co_mutex_lock(&s->lock);
> > @@ -2552,7 +2553,19 @@ static void coroutine_fn prealloc
Re: [Qemu-block] [PATCH v3 4/9] file-posix: Implement bdrv_co_copy_range
On Thu, 05/10 09:50, Stefan Hajnoczi wrote:
> On Wed, May 09, 2018 at 10:58:10PM +0800, Fam Zheng wrote:
> > +static off_t copy_file_range(int in_fd, off_t *in_off, int out_fd,
> > + off_t *out_off, size_t len, unsigned int
> > flags)
> > +{
> > +#ifdef __NR_copy_file_range
> > +return syscall(__NR_copy_file_range, in_fd, in_off, out_fd,
> > + out_off, len, flags);
> > +#else
> > +errno = ENOSYS;
> > +return -1;
> > +#endif
> > +}
> > +
> > +static ssize_t handle_aiocb_copy_range(RawPosixAIOData *aiocb)
> > +{
> > +uint64_t bytes = aiocb->aio_nbytes;
> > +off_t in_off = aiocb->aio_offset;
> > +off_t out_off = aiocb->aio_offset2;
> > +
> > +while (bytes) {
> > +ssize_t ret = copy_file_range(aiocb->aio_fildes, &in_off,
> > + aiocb->aio_fd2, &out_off,
> > + bytes, 0);
> > +if (ret == -EINTR) {
> > +continue;
> > +}
> > +if (ret < 0) {
> > +return -errno;
>
> Convert ENOSYS to ENOTSUP?
Sounds good.
