* [Qemu-devel] [PATCH v5 1/6] qapi: Add ImageInfoSpecific type
2013-09-23 12:09 [Qemu-devel] [PATCH v5 0/6] Provide additional info through qemu-img info Max Reitz
@ 2013-09-23 12:09 ` Max Reitz
2013-09-30 16:04 ` Eric Blake
2013-09-23 12:09 ` [Qemu-devel] [PATCH v5 2/6] block: Add bdrv_get_specific_info Max Reitz
` (4 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Max Reitz @ 2013-09-23 12:09 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Fam Zheng, Stefan Hajnoczi, Max Reitz
Add a new type ImageInfoSpecific as a union for image format specific
information in ImageInfo.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
qapi-schema.json | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/qapi-schema.json b/qapi-schema.json
index 145eca8..cbad705 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -210,6 +210,18 @@
'vm-clock-sec': 'int', 'vm-clock-nsec': 'int' } }
##
+# @ImageInfoSpecific:
+#
+# A discriminated record of image format specific information structures.
+#
+# Since: 1.7
+##
+
+{ 'union': 'ImageInfoSpecific',
+ 'data': {
+ } }
+
+##
# @ImageInfo:
#
# Information about a QEMU image file
@@ -238,6 +250,9 @@
#
# @backing-image: #optional info of the backing image (since 1.6)
#
+# @info-string: #optional string supplying additional format-specific
+# information (since 1.7)
+#
# Since: 1.3
#
##
@@ -248,7 +263,8 @@
'*cluster-size': 'int', '*encrypted': 'bool',
'*backing-filename': 'str', '*full-backing-filename': 'str',
'*backing-filename-format': 'str', '*snapshots': ['SnapshotInfo'],
- '*backing-image': 'ImageInfo' } }
+ '*backing-image': 'ImageInfo',
+ '*format-specific': 'ImageInfoSpecific' } }
##
# @ImageCheck:
--
1.8.3.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH v5 1/6] qapi: Add ImageInfoSpecific type
2013-09-23 12:09 ` [Qemu-devel] [PATCH v5 1/6] qapi: Add ImageInfoSpecific type Max Reitz
@ 2013-09-30 16:04 ` Eric Blake
0 siblings, 0 replies; 15+ messages in thread
From: Eric Blake @ 2013-09-30 16:04 UTC (permalink / raw)
To: Max Reitz; +Cc: Kevin Wolf, Fam Zheng, qemu-devel, Stefan Hajnoczi
[-- Attachment #1: Type: text/plain, Size: 703 bytes --]
On 09/23/2013 06:09 AM, Max Reitz wrote:
> Add a new type ImageInfoSpecific as a union for image format specific
> information in ImageInfo.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> qapi-schema.json | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
> @@ -238,6 +250,9 @@
> #
> # @backing-image: #optional info of the backing image (since 1.6)
> #
> +# @info-string: #optional string supplying additional format-specific
s/string/struct/
With that one word fixed, I'm fine with:
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH v5 2/6] block: Add bdrv_get_specific_info
2013-09-23 12:09 [Qemu-devel] [PATCH v5 0/6] Provide additional info through qemu-img info Max Reitz
2013-09-23 12:09 ` [Qemu-devel] [PATCH v5 1/6] qapi: Add ImageInfoSpecific type Max Reitz
@ 2013-09-23 12:09 ` Max Reitz
2013-09-30 16:09 ` Eric Blake
2013-09-23 12:09 ` [Qemu-devel] [PATCH v5 3/6] block/qapi: Human-readable ImageInfoSpecific dump Max Reitz
` (3 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Max Reitz @ 2013-09-23 12:09 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Fam Zheng, Stefan Hajnoczi, Max Reitz
Add a function for retrieving an ImageInfoSpecific object from a block
driver.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block.c | 9 +++++++++
block/qapi.c | 3 +++
include/block/block.h | 1 +
include/block/block_int.h | 1 +
4 files changed, 14 insertions(+)
diff --git a/block.c b/block.c
index a2ea39a..4360767 100644
--- a/block.c
+++ b/block.c
@@ -3340,6 +3340,15 @@ int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
return drv->bdrv_get_info(bs, bdi);
}
+ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs)
+{
+ BlockDriver *drv = bs->drv;
+ if (drv && drv->bdrv_get_specific_info) {
+ return drv->bdrv_get_specific_info(bs);
+ }
+ return NULL;
+}
+
int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf,
int64_t pos, int size)
{
diff --git a/block/qapi.c b/block/qapi.c
index 782051c..ab1dd24 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -134,6 +134,9 @@ void bdrv_query_image_info(BlockDriverState *bs,
info->dirty_flag = bdi.is_dirty;
info->has_dirty_flag = true;
}
+ info->format_specific = bdrv_get_specific_info(bs);
+ info->has_format_specific = info->format_specific != NULL;
+
backing_filename = bs->backing_file;
if (backing_filename[0] != '\0') {
info->backing_filename = g_strdup(backing_filename);
diff --git a/include/block/block.h b/include/block/block.h
index f808550..e265124 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -335,6 +335,7 @@ int bdrv_get_flags(BlockDriverState *bs);
int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
const uint8_t *buf, int nb_sectors);
int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);
+ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs);
void bdrv_round_to_clusters(BlockDriverState *bs,
int64_t sector_num, int nb_sectors,
int64_t *cluster_sector_num,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 3eeb6fe..5de5b0e 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -162,6 +162,7 @@ struct BlockDriver {
int (*bdrv_snapshot_load_tmp)(BlockDriverState *bs,
const char *snapshot_name);
int (*bdrv_get_info)(BlockDriverState *bs, BlockDriverInfo *bdi);
+ ImageInfoSpecific *(*bdrv_get_specific_info)(BlockDriverState *bs);
int (*bdrv_save_vmstate)(BlockDriverState *bs, QEMUIOVector *qiov,
int64_t pos);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [Qemu-devel] [PATCH v5 3/6] block/qapi: Human-readable ImageInfoSpecific dump
2013-09-23 12:09 [Qemu-devel] [PATCH v5 0/6] Provide additional info through qemu-img info Max Reitz
2013-09-23 12:09 ` [Qemu-devel] [PATCH v5 1/6] qapi: Add ImageInfoSpecific type Max Reitz
2013-09-23 12:09 ` [Qemu-devel] [PATCH v5 2/6] block: Add bdrv_get_specific_info Max Reitz
@ 2013-09-23 12:09 ` Max Reitz
2013-09-30 16:18 ` Eric Blake
2013-09-23 12:09 ` [Qemu-devel] [PATCH v5 4/6] qcow2: Add support for ImageInfoSpecific Max Reitz
` (2 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Max Reitz @ 2013-09-23 12:09 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Fam Zheng, Stefan Hajnoczi, Max Reitz
Add a function for generically dumping the ImageInfoSpecific information
in a human-readable format to block/qapi.c.
Use this function in bdrv_image_info_dump and qemu-io-cmds.c:info_f to
allow qemu-img info resp. qemu-io -c info to print that format specific
information.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/qapi.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++++++
include/block/qapi.h | 2 +
qemu-io-cmds.c | 9 ++++
3 files changed, 132 insertions(+)
diff --git a/block/qapi.c b/block/qapi.c
index ab1dd24..e7d1591 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -25,6 +25,9 @@
#include "block/qapi.h"
#include "block/block_int.h"
#include "qmp-commands.h"
+#include "qapi-visit.h"
+#include "qapi/qmp-output-visitor.h"
+#include "qapi/qmp/types.h"
/*
* Returns 0 on success, with *p_list either set to describe snapshot
@@ -426,6 +429,119 @@ void bdrv_snapshot_dump(fprintf_function func_fprintf, void *f,
}
}
+static void dump_qdict(fprintf_function func_fprintf, void *f, int indentation,
+ QDict *dict);
+static void dump_qlist(fprintf_function func_fprintf, void *f, int indentation,
+ QList *list);
+
+static void dump_qobject(fprintf_function func_fprintf, void *f,
+ int comp_indent, QObject *obj)
+{
+ switch (qobject_type(obj)) {
+ case QTYPE_QINT: {
+ QInt *value = qobject_to_qint(obj);
+ func_fprintf(f, "%" PRId64, qint_get_int(value));
+ break;
+ }
+ case QTYPE_QSTRING: {
+ QString *value = qobject_to_qstring(obj);
+ func_fprintf(f, "%s", qstring_get_str(value));
+ break;
+ }
+ case QTYPE_QDICT: {
+ QDict *value = qobject_to_qdict(obj);
+ dump_qdict(func_fprintf, f, comp_indent, value);
+ break;
+ }
+ case QTYPE_QLIST: {
+ QList *value = qobject_to_qlist(obj);
+ dump_qlist(func_fprintf, f, comp_indent, value);
+ break;
+ }
+ case QTYPE_QFLOAT: {
+ QFloat *value = qobject_to_qfloat(obj);
+ func_fprintf(f, "%g", qfloat_get_double(value));
+ break;
+ }
+ case QTYPE_QBOOL: {
+ QBool *value = qobject_to_qbool(obj);
+ func_fprintf(f, "%s", qbool_get_int(value) ? "true" : "false");
+ break;
+ }
+ case QTYPE_QERROR: {
+ QString *value = qerror_human((QError *)obj);
+ func_fprintf(f, "%s", qstring_get_str(value));
+ break;
+ }
+ case QTYPE_NONE:
+ break;
+ case QTYPE_MAX:
+ default:
+ abort();
+ }
+}
+
+static void dump_qlist(fprintf_function func_fprintf, void *f, int indentation,
+ QList *list)
+{
+ const QListEntry *entry;
+ int i = 0;
+
+ for (entry = qlist_first(list); entry; entry = qlist_next(entry), i++) {
+ qtype_code type = qobject_type(entry->value);
+ bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
+ const char *format = composite ? "%*s[%i]:\n" : "%*s[%i]: ";
+
+ func_fprintf(f, format, indentation * 4, "", i);
+ dump_qobject(func_fprintf, f, indentation + 1, entry->value);
+ if (!composite) {
+ func_fprintf(f, "\n");
+ }
+ }
+}
+
+static void dump_qdict(fprintf_function func_fprintf, void *f, int indentation,
+ QDict *dict)
+{
+ const QDictEntry *entry;
+
+ for (entry = qdict_first(dict); entry; entry = qdict_next(dict, entry)) {
+ qtype_code type = qobject_type(entry->value);
+ bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
+ const char *format = composite ? "%*s%s:\n" : "%*s%s: ";
+ char key[strlen(entry->key) + 1];
+ int i;
+
+ /* replace dashes with spaces in key (variable) names */
+ for (i = 0; entry->key[i]; i++) {
+ key[i] = entry->key[i] == '-' ? ' ' : entry->key[i];
+ }
+ key[i] = 0;
+
+ func_fprintf(f, format, indentation * 4, "", key);
+ dump_qobject(func_fprintf, f, indentation + 1, entry->value);
+ if (!composite) {
+ func_fprintf(f, "\n");
+ }
+ }
+}
+
+void bdrv_image_info_specific_dump(fprintf_function func_fprintf, void *f,
+ ImageInfoSpecific *info_spec)
+{
+ Error *local_err = NULL;
+ QmpOutputVisitor *ov = qmp_output_visitor_new();
+ QObject *obj, *data;
+
+ visit_type_ImageInfoSpecific(qmp_output_get_visitor(ov), &info_spec, NULL,
+ &local_err);
+ obj = qmp_output_get_qobject(ov);
+ assert(qobject_type(obj) == QTYPE_QDICT);
+ data = qdict_get(qobject_to_qdict(obj), "data");
+ dump_qobject(func_fprintf, f, 0, data);
+ qmp_output_visitor_cleanup(ov);
+}
+
void bdrv_image_info_dump(fprintf_function func_fprintf, void *f,
ImageInfo *info)
{
@@ -496,4 +612,9 @@ void bdrv_image_info_dump(fprintf_function func_fprintf, void *f,
func_fprintf(f, "\n");
}
}
+
+ if (info->has_format_specific) {
+ func_fprintf(f, "Format specific information:\n");
+ bdrv_image_info_specific_dump(func_fprintf, f, info->format_specific);
+ }
}
diff --git a/include/block/qapi.h b/include/block/qapi.h
index 0496cc9..9518ee4 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -42,6 +42,8 @@ BlockStats *bdrv_query_stats(const BlockDriverState *bs);
void bdrv_snapshot_dump(fprintf_function func_fprintf, void *f,
QEMUSnapshotInfo *sn);
+void bdrv_image_info_specific_dump(fprintf_function func_fprintf, void *f,
+ ImageInfoSpecific *info_spec);
void bdrv_image_info_dump(fprintf_function func_fprintf, void *f,
ImageInfo *info);
#endif
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 8565d49..667f4e4 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -10,6 +10,7 @@
#include "qemu-io.h"
#include "block/block_int.h"
+#include "block/qapi.h"
#include "qemu/main-loop.h"
#define CMD_NOFILE_OK 0x01
@@ -1678,6 +1679,7 @@ static const cmdinfo_t length_cmd = {
static int info_f(BlockDriverState *bs, int argc, char **argv)
{
BlockDriverInfo bdi;
+ ImageInfoSpecific *spec_info;
char s1[64], s2[64];
int ret;
@@ -1699,6 +1701,13 @@ static int info_f(BlockDriverState *bs, int argc, char **argv)
printf("cluster size: %s\n", s1);
printf("vm state offset: %s\n", s2);
+ spec_info = bdrv_get_specific_info(bs);
+ if (spec_info) {
+ printf("Format specific information:\n");
+ bdrv_image_info_specific_dump(fprintf, stdout, spec_info);
+ qapi_free_ImageInfoSpecific(spec_info);
+ }
+
return 0;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH v5 3/6] block/qapi: Human-readable ImageInfoSpecific dump
2013-09-23 12:09 ` [Qemu-devel] [PATCH v5 3/6] block/qapi: Human-readable ImageInfoSpecific dump Max Reitz
@ 2013-09-30 16:18 ` Eric Blake
0 siblings, 0 replies; 15+ messages in thread
From: Eric Blake @ 2013-09-30 16:18 UTC (permalink / raw)
To: Max Reitz; +Cc: Kevin Wolf, Fam Zheng, qemu-devel, Stefan Hajnoczi
[-- Attachment #1: Type: text/plain, Size: 1867 bytes --]
On 09/23/2013 06:09 AM, Max Reitz wrote:
> Add a function for generically dumping the ImageInfoSpecific information
> in a human-readable format to block/qapi.c.
>
> Use this function in bdrv_image_info_dump and qemu-io-cmds.c:info_f to
> allow qemu-img info resp. qemu-io -c info to print that format specific
> information.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> block/qapi.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++++++
> include/block/qapi.h | 2 +
> qemu-io-cmds.c | 9 ++++
> 3 files changed, 132 insertions(+)
>
> +
> +static void dump_qdict(fprintf_function func_fprintf, void *f, int indentation,
> + QDict *dict)
> +{
> + const QDictEntry *entry;
> +
> + for (entry = qdict_first(dict); entry; entry = qdict_next(dict, entry)) {
> + qtype_code type = qobject_type(entry->value);
> + bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
> + const char *format = composite ? "%*s%s:\n" : "%*s%s: ";
> + char key[strlen(entry->key) + 1];
> + int i;
> +
> + /* replace dashes with spaces in key (variable) names */
> + for (i = 0; entry->key[i]; i++) {
> + key[i] = entry->key[i] == '-' ? ' ' : entry->key[i];
I wonder if this should replace underscores, too. For now, this
function is only being used on types that happen to use - in key names,
but it seems generic enough that we may want to use it on older types
that followed older conventions. But I won't force a respin for just
that (if someone DOES reuse this function in the future, they can tweak
underscores at that time, if you don't do it now).
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH v5 4/6] qcow2: Add support for ImageInfoSpecific
2013-09-23 12:09 [Qemu-devel] [PATCH v5 0/6] Provide additional info through qemu-img info Max Reitz
` (2 preceding siblings ...)
2013-09-23 12:09 ` [Qemu-devel] [PATCH v5 3/6] block/qapi: Human-readable ImageInfoSpecific dump Max Reitz
@ 2013-09-23 12:09 ` Max Reitz
2013-09-30 16:25 ` Eric Blake
2013-09-23 12:09 ` [Qemu-devel] [PATCH v5 5/6] qemu-iotests: Discard specific info in _img_info Max Reitz
2013-09-23 12:09 ` [Qemu-devel] [PATCH v5 6/6] qemu-iotests: Additional info from qemu-img info Max Reitz
5 siblings, 1 reply; 15+ messages in thread
From: Max Reitz @ 2013-09-23 12:09 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Fam Zheng, Stefan Hajnoczi, Max Reitz
Add a new ImageInfoSpecificQCow2 type as a subtype of ImageInfoSpecific.
This contains the compatibility level as a string and an optional
lazy_refcounts boolean (optional means mandatory for compat >= 1.1 and
not available for compat == 0.10).
Also, add qcow2_get_specific_info, which returns this information.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/qcow2.c | 19 +++++++++++++++++++
qapi-schema.json | 16 ++++++++++++++++
2 files changed, 35 insertions(+)
diff --git a/block/qcow2.c b/block/qcow2.c
index 318d95d..f080a8a 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1810,6 +1810,24 @@ static int qcow2_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
return 0;
}
+static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
+{
+ BDRVQcowState *s = bs->opaque;
+ ImageInfoSpecific *spec_info = g_new0(ImageInfoSpecific, 1);
+
+ spec_info->kind = IMAGE_INFO_SPECIFIC_KIND_QCOW2;
+ spec_info->qcow2 = g_new0(ImageInfoSpecificQCow2, 1);
+ if (s->qcow_version == 2) {
+ spec_info->qcow2->compat = g_strdup("0.10");
+ } else if (s->qcow_version == 3) {
+ spec_info->qcow2->compat = g_strdup("1.1");
+ spec_info->qcow2->lazy_refcounts = s->use_lazy_refcounts;
+ spec_info->qcow2->has_lazy_refcounts = true;
+ }
+
+ return spec_info;
+}
+
#if 0
static void dump_refcounts(BlockDriverState *bs)
{
@@ -2130,6 +2148,7 @@ static BlockDriver bdrv_qcow2 = {
.bdrv_snapshot_list = qcow2_snapshot_list,
.bdrv_snapshot_load_tmp = qcow2_snapshot_load_tmp,
.bdrv_get_info = qcow2_get_info,
+ .bdrv_get_specific_info = qcow2_get_specific_info,
.bdrv_save_vmstate = qcow2_save_vmstate,
.bdrv_load_vmstate = qcow2_load_vmstate,
diff --git a/qapi-schema.json b/qapi-schema.json
index cbad705..d697da2 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -210,6 +210,21 @@
'vm-clock-sec': 'int', 'vm-clock-nsec': 'int' } }
##
+# @ImageInfoSpecificQCow2:
+#
+# @compat: compatibility level
+#
+# @lazy-refcounts: #optional on or off; only valid for compat >= 1.1
+#
+# Since: 1.7
+##
+{ 'type': 'ImageInfoSpecificQCow2',
+ 'data': {
+ 'compat': 'str',
+ '*lazy-refcounts': 'bool'
+ } }
+
+##
# @ImageInfoSpecific:
#
# A discriminated record of image format specific information structures.
@@ -219,6 +234,7 @@
{ 'union': 'ImageInfoSpecific',
'data': {
+ 'qcow2': 'ImageInfoSpecificQCow2'
} }
##
--
1.8.3.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH v5 4/6] qcow2: Add support for ImageInfoSpecific
2013-09-23 12:09 ` [Qemu-devel] [PATCH v5 4/6] qcow2: Add support for ImageInfoSpecific Max Reitz
@ 2013-09-30 16:25 ` Eric Blake
0 siblings, 0 replies; 15+ messages in thread
From: Eric Blake @ 2013-09-30 16:25 UTC (permalink / raw)
To: Max Reitz; +Cc: Kevin Wolf, Fam Zheng, qemu-devel, Stefan Hajnoczi
[-- Attachment #1: Type: text/plain, Size: 721 bytes --]
On 09/23/2013 06:09 AM, Max Reitz wrote:
> Add a new ImageInfoSpecificQCow2 type as a subtype of ImageInfoSpecific.
> This contains the compatibility level as a string and an optional
> lazy_refcounts boolean (optional means mandatory for compat >= 1.1 and
> not available for compat == 0.10).
>
> Also, add qcow2_get_specific_info, which returns this information.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> block/qcow2.c | 19 +++++++++++++++++++
> qapi-schema.json | 16 ++++++++++++++++
> 2 files changed, 35 insertions(+)
>
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH v5 5/6] qemu-iotests: Discard specific info in _img_info
2013-09-23 12:09 [Qemu-devel] [PATCH v5 0/6] Provide additional info through qemu-img info Max Reitz
` (3 preceding siblings ...)
2013-09-23 12:09 ` [Qemu-devel] [PATCH v5 4/6] qcow2: Add support for ImageInfoSpecific Max Reitz
@ 2013-09-23 12:09 ` Max Reitz
2013-09-30 17:14 ` Eric Blake
2013-09-23 12:09 ` [Qemu-devel] [PATCH v5 6/6] qemu-iotests: Additional info from qemu-img info Max Reitz
5 siblings, 1 reply; 15+ messages in thread
From: Max Reitz @ 2013-09-23 12:09 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Fam Zheng, Stefan Hajnoczi, Max Reitz
In _img_info, filter out additional information specific to the image
format provided by qemu-img info, since tests designed for multiple
image formats would produce different outputs for every image format
else.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/common.rc | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 28b39e4..12d8882 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -181,12 +181,29 @@ _check_test_img()
_img_info()
{
+ discard=0
$QEMU_IMG info "$@" $TEST_IMG 2>&1 | \
sed -e "s#$IMGPROTO:$TEST_DIR#TEST_DIR#g" \
-e "s#$TEST_DIR#TEST_DIR#g" \
-e "s#$IMGFMT#IMGFMT#g" \
-e "/^disk size:/ D" \
- -e "/actual-size/ D"
+ -e "/actual-size/ D" | \
+ while IFS='' read line; do
+ if [ "$line" == "Format specific information:" ]; then
+ discard=1
+ elif [ "`echo "$line" | sed -e 's/^ *//'`" == '"format-specific": {' ]; then
+ discard=2
+ json_indent="`echo "$line" | sed -e 's/^\( *\).*$/\1/'`"
+ fi
+ if [ $discard == 0 ]; then
+ echo "$line"
+ elif [ $discard == 1 -a -z "$line" ]; then
+ echo
+ discard=0
+ elif [ $discard == 2 -a "`echo "$line" | sed -e 's/ *$//'`" == "${json_indent}}," ]; then
+ discard=0
+ fi
+ done
}
_get_pids_by_name()
--
1.8.3.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH v5 5/6] qemu-iotests: Discard specific info in _img_info
2013-09-23 12:09 ` [Qemu-devel] [PATCH v5 5/6] qemu-iotests: Discard specific info in _img_info Max Reitz
@ 2013-09-30 17:14 ` Eric Blake
2013-10-01 8:25 ` Max Reitz
0 siblings, 1 reply; 15+ messages in thread
From: Eric Blake @ 2013-09-30 17:14 UTC (permalink / raw)
To: Max Reitz; +Cc: Kevin Wolf, Fam Zheng, qemu-devel, Stefan Hajnoczi
[-- Attachment #1: Type: text/plain, Size: 3975 bytes --]
On 09/23/2013 06:09 AM, Max Reitz wrote:
> In _img_info, filter out additional information specific to the image
> format provided by qemu-img info, since tests designed for multiple
> image formats would produce different outputs for every image format
> else.
s/else/otherwise/
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> tests/qemu-iotests/common.rc | 19 ++++++++++++++++++-
> 1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
> index 28b39e4..12d8882 100644
> --- a/tests/qemu-iotests/common.rc
> +++ b/tests/qemu-iotests/common.rc
> @@ -181,12 +181,29 @@ _check_test_img()
>
> _img_info()
> {
> + discard=0
> $QEMU_IMG info "$@" $TEST_IMG 2>&1 | \
> sed -e "s#$IMGPROTO:$TEST_DIR#TEST_DIR#g" \
> -e "s#$TEST_DIR#TEST_DIR#g" \
> -e "s#$IMGFMT#IMGFMT#g" \
> -e "/^disk size:/ D" \
> - -e "/actual-size/ D"
> + -e "/actual-size/ D" | \
> + while IFS='' read line; do
> + if [ "$line" == "Format specific information:" ]; then
[ ... == ...] is a bashism (thank goodness this is already a bash
script); but I generally prefer you either stick to portable syntax:
if [ "$line" = "Format specific information:" ]
or make it obvious that you know you are using bash:
if [[ $line == "Format specific information:" ]]
> + discard=1
> + elif [ "`echo "$line" | sed -e 's/^ *//'`" == '"format-specific": {' ]; then
Use $(), not ``.
This script is already a bash script; why not exploit that and avoid a fork:
elif [[ $line =~ '"format-specific": {' ]]
> + discard=2
> + json_indent="`echo "$line" | sed -e 's/^\( *\).*$/\1/'`"
Use $(), not ``.
Exploit bash to avoid a fork:
json_indent=${line%%[! ]*}
> + fi
> + if [ $discard == 0 ]; then
Again, I don't like the bashism of [ == ].
> + echo "$line"
> + elif [ $discard == 1 -a -z "$line" ]; then
[ ... -a ... ] is flat out non-portable. Even when you are already
requiring bash. For example:
[ "$str1" -a "$str2" ]
gives status 0 for most pairs of non-empty strings, but could give
status 1 for str1="!" and str2=".". Using a bashism, on the other hand,
is unambiguous:
elif [[ $discard == 1 && ! $line ]]
> + echo
> + discard=0
> + elif [ $discard == 2 -a "`echo "$line" | sed -e 's/ *$//'`" == "${json_indent}}," ]; then
Huh? If we detected json output, then compare whether the current line
with trailing whitespace stripped is now identical to $json_indent; but
based on the above, you set $json_indent to contain JUST whitespace. A
line with trailing whitespace removed will NEVER match a variable that
contains just whitespace, so this condition will never trigger. Not to
mention the deprecated `` and non-portable use of == and -a inside [].
> + discard=0
> + fi
> + done
For the human output case, sed can already do everything your 'while
read' loop did:
sed ...
-e "/^disk size:/ D" \
-e "/actual-size/ D" \
-e "/Format specific information/,/^$/ D"
but for the JSON output case, while I'm sure that sed could probably be
coerced into stripping lines until the first line that does not have at
least as much indentation as the line containing "format-specific", the
resulting script wouldn't be very pretty to read (I couldn't quickly
produce one, at any rate - maybe Paolo has more expertise at writing
arcane sed scripts).
This patch might be easier to review if you provided a sample in the
commit message of what input is being stripped by this patch.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH v5 5/6] qemu-iotests: Discard specific info in _img_info
2013-09-30 17:14 ` Eric Blake
@ 2013-10-01 8:25 ` Max Reitz
0 siblings, 0 replies; 15+ messages in thread
From: Max Reitz @ 2013-10-01 8:25 UTC (permalink / raw)
To: Eric Blake; +Cc: Kevin Wolf, Fam Zheng, qemu-devel, Stefan Hajnoczi
On 2013-09-30 19:14, Eric Blake wrote:
> On 09/23/2013 06:09 AM, Max Reitz wrote:
>> In _img_info, filter out additional information specific to the image
>> format provided by qemu-img info, since tests designed for multiple
>> image formats would produce different outputs for every image format
>> else.
> s/else/otherwise/
>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> tests/qemu-iotests/common.rc | 19 ++++++++++++++++++-
>> 1 file changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
>> index 28b39e4..12d8882 100644
>> --- a/tests/qemu-iotests/common.rc
>> +++ b/tests/qemu-iotests/common.rc
>> @@ -181,12 +181,29 @@ _check_test_img()
>>
>> _img_info()
>> {
>> + discard=0
>> $QEMU_IMG info "$@" $TEST_IMG 2>&1 | \
>> sed -e "s#$IMGPROTO:$TEST_DIR#TEST_DIR#g" \
>> -e "s#$TEST_DIR#TEST_DIR#g" \
>> -e "s#$IMGFMT#IMGFMT#g" \
>> -e "/^disk size:/ D" \
>> - -e "/actual-size/ D"
>> + -e "/actual-size/ D" | \
>> + while IFS='' read line; do
>> + if [ "$line" == "Format specific information:" ]; then
> [ ... == ...] is a bashism (thank goodness this is already a bash
> script); but I generally prefer you either stick to portable syntax:
>
> if [ "$line" = "Format specific information:" ]
>
> or make it obvious that you know you are using bash:
>
> if [[ $line == "Format specific information:" ]]
>
>> + discard=1
>> + elif [ "`echo "$line" | sed -e 's/^ *//'`" == '"format-specific": {' ]; then
> Use $(), not ``.
>
> This script is already a bash script; why not exploit that and avoid a fork:
>
> elif [[ $line =~ '"format-specific": {' ]]
>
>> + discard=2
>> + json_indent="`echo "$line" | sed -e 's/^\( *\).*$/\1/'`"
> Use $(), not ``.
>
> Exploit bash to avoid a fork:
>
> json_indent=${line%%[! ]*}
>
>> + fi
>> + if [ $discard == 0 ]; then
> Again, I don't like the bashism of [ == ].
>
>> + echo "$line"
>> + elif [ $discard == 1 -a -z "$line" ]; then
> [ ... -a ... ] is flat out non-portable. Even when you are already
> requiring bash. For example:
>
> [ "$str1" -a "$str2" ]
>
> gives status 0 for most pairs of non-empty strings, but could give
> status 1 for str1="!" and str2=".". Using a bashism, on the other hand,
> is unambiguous:
>
> elif [[ $discard == 1 && ! $line ]]
Okay, I'll rewrite all these to use bash syntax.
>> + echo
>> + discard=0
>> + elif [ $discard == 2 -a "`echo "$line" | sed -e 's/ *$//'`" == "${json_indent}}," ]; then
> Huh? If we detected json output, then compare whether the current line
> with trailing whitespace stripped is now identical to $json_indent
Take a closer look: "${json_indent}}," is not "${json_indent}", mind the
trailing "}," ;)
Therefore, this matches when the current line is the ending brace of the
"format-specific" structure.
>> + discard=0
>> + fi
>> + done
> For the human output case, sed can already do everything your 'while
> read' loop did:
>
> sed ...
> -e "/^disk size:/ D" \
> -e "/actual-size/ D" \
> -e "/Format specific information/,/^$/ D"
Oh, nice; sorry, but multiline sed regexes are something I know
basically nothing about. I'd leave the script as it is for now, anyway
(while implementing your remarks), because of the JSON filter and since
I don't think this to be the bottleneck in test cases.
In your review to patch 6 you asked whether the JSON filter is actually
necessary: Test 043 is a shell script which simply queries the JSON
output (in addition to the human-readable version) and is valid for
multiple image formats (qcow2 and qed), therefore the format specific
information has to be filtered out, both for human-readable and JSON output.
Max
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH v5 6/6] qemu-iotests: Additional info from qemu-img info
2013-09-23 12:09 [Qemu-devel] [PATCH v5 0/6] Provide additional info through qemu-img info Max Reitz
` (4 preceding siblings ...)
2013-09-23 12:09 ` [Qemu-devel] [PATCH v5 5/6] qemu-iotests: Discard specific info in _img_info Max Reitz
@ 2013-09-23 12:09 ` Max Reitz
2013-09-30 17:19 ` Eric Blake
5 siblings, 1 reply; 15+ messages in thread
From: Max Reitz @ 2013-09-23 12:09 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Fam Zheng, Stefan Hajnoczi, Max Reitz
Add a test for the additional information now provided by qemu-img info
when used on qcow2 images.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/065 | 72 ++++++++++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/065.out | 22 ++++++++++++++
tests/qemu-iotests/group | 1 +
3 files changed, 95 insertions(+)
create mode 100755 tests/qemu-iotests/065
create mode 100644 tests/qemu-iotests/065.out
diff --git a/tests/qemu-iotests/065 b/tests/qemu-iotests/065
new file mode 100755
index 0000000..5c56b56
--- /dev/null
+++ b/tests/qemu-iotests/065
@@ -0,0 +1,72 @@
+#!/bin/bash
+#
+# Test for additional information emitted by qemu-img info on qcow2
+# images
+#
+# Copyright (C) 2013 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=mreitz@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1 # failure is the default!
+
+_cleanup()
+{
+ _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+# This tests qocw2-specific low-level functionality
+_supported_fmt qcow2
+_supported_proto generic
+_supported_os Linux
+
+IMG_SIZE=64M
+
+echo
+echo "=== Testing qcow2 image with -o compat=0.10 ==="
+echo
+IMGOPTS="compat=0.10" _make_test_img $IMG_SIZE
+# don't use _img_info, since that function will filter out the
+# additional information we're about to test for
+$QEMU_IMG info "$TEST_IMG" | sed -n '/^Format specific information:$/,$p'
+
+echo
+echo "=== Testing qcow2 image with -o compat=1.1,lazy_refcounts=off ==="
+echo
+IMGOPTS="compat=1.1,lazy_refcounts=off" _make_test_img $IMG_SIZE
+$QEMU_IMG info "$TEST_IMG" | sed -n '/^Format specific information:$/,$p'
+
+echo
+echo "=== Testing qcow2 image with -o compat=1.1,lazy_refcounts=on ==="
+echo
+IMGOPTS="compat=1.1,lazy_refcounts=on" _make_test_img $IMG_SIZE
+$QEMU_IMG info "$TEST_IMG" | sed -n '/^Format specific information:$/,$p'
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/065.out b/tests/qemu-iotests/065.out
new file mode 100644
index 0000000..4dd7f2b
--- /dev/null
+++ b/tests/qemu-iotests/065.out
@@ -0,0 +1,22 @@
+QA output created by 065
+
+=== Testing qcow2 image with -o compat=0.10 ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+Format specific information:
+compat: 0.10
+
+=== Testing qcow2 image with -o compat=1.1,lazy_refcounts=off ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+Format specific information:
+compat: 1.1
+lazy refcounts: false
+
+=== Testing qcow2 image with -o compat=1.1,lazy_refcounts=on ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+Format specific information:
+compat: 1.1
+lazy refcounts: true
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 1ad02e5..f1a68b0 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -69,3 +69,4 @@
061 rw auto
062 rw auto
063 rw auto
+065 rw auto
--
1.8.3.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH v5 6/6] qemu-iotests: Additional info from qemu-img info
2013-09-23 12:09 ` [Qemu-devel] [PATCH v5 6/6] qemu-iotests: Additional info from qemu-img info Max Reitz
@ 2013-09-30 17:19 ` Eric Blake
2013-10-01 9:19 ` Max Reitz
0 siblings, 1 reply; 15+ messages in thread
From: Eric Blake @ 2013-09-30 17:19 UTC (permalink / raw)
To: Max Reitz; +Cc: Kevin Wolf, Fam Zheng, qemu-devel, Stefan Hajnoczi
[-- Attachment #1: Type: text/plain, Size: 1647 bytes --]
On 09/23/2013 06:09 AM, Max Reitz wrote:
> Add a test for the additional information now provided by qemu-img info
> when used on qcow2 images.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> tests/qemu-iotests/065 | 72 ++++++++++++++++++++++++++++++++++++++++++++++
> tests/qemu-iotests/065.out | 22 ++++++++++++++
> tests/qemu-iotests/group | 1 +
> 3 files changed, 95 insertions(+)
> create mode 100755 tests/qemu-iotests/065
> create mode 100644 tests/qemu-iotests/065.out
This patch only tests human output, not JSON.
> +# creator
> +owner=mreitz@redhat.com
> +
> +seq=`basename $0`
> +echo "QA output created by $seq"
> +
> +here=`pwd`
Not your fault (copy-and-paste from other tests), but as long as we are
requiring bash, $PWD is much faster than `pwd`, and $() is nicer than ``
where we don't have even faster shortcuts like $PWD.
> +=== Testing qcow2 image with -o compat=0.10 ===
> +
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> +Format specific information:
> +compat: 0.10
Should we be indenting the human output, to make it obvious how many
remaining fields are being output as a result of format specific
information?
I'm still not happy with 5/6 in its current form, but can live with this
patch as-is if we don't bother with testing JSON form. Does 5/6 even
need to worry about stripping JSON form, if you aren't going to test
JSON form? Depending on the answer to that question:
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v5 6/6] qemu-iotests: Additional info from qemu-img info
2013-09-30 17:19 ` Eric Blake
@ 2013-10-01 9:19 ` Max Reitz
0 siblings, 0 replies; 15+ messages in thread
From: Max Reitz @ 2013-10-01 9:19 UTC (permalink / raw)
To: Eric Blake; +Cc: Kevin Wolf, Fam Zheng, qemu-devel, Stefan Hajnoczi
On 2013-09-30 19:19, Eric Blake wrote:
> On 09/23/2013 06:09 AM, Max Reitz wrote:
>> Add a test for the additional information now provided by qemu-img info
>> when used on qcow2 images.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> tests/qemu-iotests/065 | 72 ++++++++++++++++++++++++++++++++++++++++++++++
>> tests/qemu-iotests/065.out | 22 ++++++++++++++
>> tests/qemu-iotests/group | 1 +
>> 3 files changed, 95 insertions(+)
>> create mode 100755 tests/qemu-iotests/065
>> create mode 100644 tests/qemu-iotests/065.out
> This patch only tests human output, not JSON.
I'd not be happy at all with testing JSON output through a shell script
(or rather, without properly parsing it). Since I don't really see the
point of testing the JSON output as well: It is basically generated the
same way as the human-readable output, however, the latter uses the new
bdrv_image_info_specific_dump function, so I think the human-readable
output is actually more "error-prone" (and if something breaks the JSON
output that doesn't break other JSON tests, it should break the
human-readable output as well).
So I'll leave the JSON test out and include a note in the commit message.
>> +# creator
>> +owner=mreitz@redhat.com
>> +
>> +seq=`basename $0`
>> +echo "QA output created by $seq"
>> +
>> +here=`pwd`
> Not your fault (copy-and-paste from other tests), but as long as we are
> requiring bash, $PWD is much faster than `pwd`, and $() is nicer than ``
> where we don't have even faster shortcuts like $PWD.
Well, it's executed just once per test, so it shouldn't be that much of
a performance killer, but I'll change it anyway, thanks. ;)
>> +=== Testing qcow2 image with -o compat=0.10 ===
>> +
>> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
>> +Format specific information:
>> +compat: 0.10
> Should we be indenting the human output, to make it obvious how many
> remaining fields are being output as a result of format specific
> information?
Seems very reasonable. I'll have to change one of the other patches for
this as well, but this should be a very minor change.
> I'm still not happy with 5/6 in its current form, but can live with this
> patch as-is if we don't bother with testing JSON form. Does 5/6 even
> need to worry about stripping JSON form, if you aren't going to test
> JSON form?
As I've said in my answer to patch 5, stripping JSON isn't for this new
test, but rather for compatibility with the old tests (specifically,
test 043).
Max
^ permalink raw reply [flat|nested] 15+ messages in thread