From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:57534) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tj83N-0001Bv-BC for qemu-devel@nongnu.org; Thu, 13 Dec 2012 07:38:19 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Tj83L-0007ML-Hv for qemu-devel@nongnu.org; Thu, 13 Dec 2012 07:38:17 -0500 Received: from mx1.redhat.com ([209.132.183.28]:19256) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tj83L-0007MG-9Z for qemu-devel@nongnu.org; Thu, 13 Dec 2012 07:38:15 -0500 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id qBDCcEW6030686 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 13 Dec 2012 07:38:14 -0500 Message-ID: <50C9CC34.2040402@redhat.com> Date: Thu, 13 Dec 2012 13:38:12 +0100 From: Kevin Wolf MIME-Version: 1.0 References: <1355158885-21285-1-git-send-email-fsimonce@redhat.com> <1355158885-21285-2-git-send-email-fsimonce@redhat.com> In-Reply-To: <1355158885-21285-2-git-send-email-fsimonce@redhat.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/2] qemu-img: add json output option to the check command List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Federico Simoncelli Cc: qemu-devel@nongnu.org Am 10.12.2012 18:01, schrieb Federico Simoncelli: > This option --output=[human|json] make qemu-img check output an human > or JSON representation at the choice of the user. > > Signed-off-by: Federico Simoncelli > --- > qapi-schema.json | 38 +++++++++ > qemu-img-cmds.hx | 4 +- > qemu-img.c | 246 +++++++++++++++++++++++++++++++++++++++--------------- > qemu-img.texi | 5 +- > 4 files changed, 221 insertions(+), 72 deletions(-) > > diff --git a/qapi-schema.json b/qapi-schema.json > index 5dfa052..8877285 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -245,6 +245,44 @@ > '*backing-filename-format': 'str', '*snapshots': ['SnapshotInfo'] } } > > ## > +# @ImageCheck: > +# > +# Information about a QEMU image file check > +# > +# @filename: name of the image file checked > +# > +# @format: format of the image file checked > +# > +# @check-errors: number of unexpected errors occurred during check > +# > +# @highest-offset: #optional highest offset (in bytes) in use by the image How about adding something like "This field is present if the driver for the image format supports it" in order to explain the #optional? > +# > +# @corruptions: #optional number of corruptions found during the check > +# > +# @leaks: #optional number of leaks found during the check > +# > +# @corruptions-fixed: #optional number of corruptions fixed during the check > +# > +# @leaks-fixed: #optional number of leaks fixed during the check These four could be clarified by adding "if any" > +# > +# @total-clusters: #optional total number of clusters > +# > +# @allocated-clusters: #optional total number of allocated clusters > +# > +# @fragmented-clusters: #optional total number of fragmented clusters And here #optional is about the image format again > +# > +# Since: 1.4 > +# > +## > + > +{ 'type': 'ImageCheck', > + 'data': {'filename': 'str', 'format': 'str', 'check-errors': 'int', > + '*highest-offset': 'int', '*corruptions': 'int', '*leaks': 'int', > + '*corruptions-fixed': 'int', '*leaks-fixed': 'int', > + '*total-clusters': 'int', '*allocated-clusters': 'int', > + '*fragmented-clusters': 'int' } } > + > +## > # @StatusInfo: > # > # Information about VCPU run state > diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx > index a181363..259fc14 100644 > --- a/qemu-img-cmds.hx > +++ b/qemu-img-cmds.hx > @@ -10,9 +10,9 @@ STEXI > ETEXI > > DEF("check", img_check, > - "check [-f fmt] [-r [leaks | all]] filename") > + "check [-f fmt] [--output=ofmt] [-r [leaks | all]] filename") > STEXI > -@item check [-f @var{fmt}] [-r [leaks | all]] @var{filename} > +@item check [-f @var{fmt}] [--output=@var{ofmt}] [-r [leaks | all]] @var{filename} > ETEXI > > DEF("create", img_create, > diff --git a/qemu-img.c b/qemu-img.c > index 45c1ec1..18ba5c2 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -42,6 +42,16 @@ typedef struct img_cmd_t { > int (*handler)(int argc, char **argv); > } img_cmd_t; > > +enum { > + OPTION_OUTPUT = 256, > + OPTION_BACKING_CHAIN = 257, > +}; > + > +typedef enum OutputFormat { > + OFORMAT_JSON, > + OFORMAT_HUMAN, > +} OutputFormat; > + > /* Default to cache=writeback as data integrity is not important for qemu-tcg. */ > #define BDRV_O_FLAGS BDRV_O_CACHE_WB > #define BDRV_DEFAULT_CACHE "writeback" > @@ -370,6 +380,122 @@ out: > return 0; > } > > +static void dump_json_image_check(ImageCheck *check) > +{ > + Error *errp = NULL; > + QString *str; > + QmpOutputVisitor *ov = qmp_output_visitor_new(); > + QObject *obj; > + visit_type_ImageCheck(qmp_output_get_visitor(ov), > + &check, NULL, &errp); > + obj = qmp_output_get_qobject(ov); > + str = qobject_to_json_pretty(obj); > + assert(str != NULL); > + printf("%s\n", qstring_get_str(str)); > + qobject_decref(obj); > + qmp_output_visitor_cleanup(ov); > + QDECREF(str); > +} > + > +static void dump_human_image_check(ImageCheck *check) > +{ > + if (check->corruptions_fixed || check->leaks_fixed) { > + printf("The following inconsistencies were found and repaired:\n\n" > + " %" PRId64 " leaked clusters\n" > + " %" PRId64 " corruptions\n\n" > + "Double checking the fixed image now...\n", > + check->leaks_fixed, > + check->corruptions_fixed); > + } This message is now printed in the wrong place. It should be after the first run, but before the second one. I guess we'll have to move it to collect_image_check for this, and check explicitly for human mode there. > + > + if (!(check->corruptions || check->leaks || check->check_errors)) { > + printf("No errors were found on the image.\n"); > + } else { > + if (check->corruptions) { > + printf("\n%" PRId64 " errors were found on the image.\n" > + "Data may be corrupted, or further writes to the image " > + "may corrupt it.\n", > + check->corruptions); > + } > + > + if (check->leaks) { > + printf("\n%" PRId64 " leaked clusters were found on the image.\n" > + "This means waste of disk space, but no harm to data.\n", > + check->leaks); > + } > + > + if (check->check_errors) { > + printf("\n%" PRId64 " internal errors have occurred during the check.\n", > + check->check_errors); > + } > + } > + > + if (check->total_clusters != 0 && check->allocated_clusters != 0) { > + printf("%" PRId64 "/%" PRId64 "= %0.2f%% allocated, %0.2f%% fragmented\n", > + check->allocated_clusters, check->total_clusters, > + check->allocated_clusters * 100.0 / check->total_clusters, > + check->fragmented_clusters * 100.0 / check->allocated_clusters); > + } > + > + if (check->highest_offset) { > + printf("Highest offset in use: %" PRId64 "\n", check->highest_offset); > + } > +} > + > +static int collect_image_check(BlockDriverState *bs, > + ImageCheck *check, > + const char *filename, > + const char *fmt, > + int fix) > +{ > + int ret; > + BdrvCheckResult result; > + > + ret = bdrv_check(bs, &result, fix); > + > + if (ret < 0) { > + return ret; > + } > + > + check->filename = g_strdup(filename); > + check->format = g_strdup(bdrv_get_format_name(bs)); > + check->check_errors = result.check_errors; > + > + check->corruptions = result.corruptions; > + check->has_corruptions = result.corruptions != 0; > + check->leaks = result.leaks; > + check->has_leaks = result.leaks != 0; > + check->corruptions_fixed = result.corruptions_fixed; > + check->has_corruptions_fixed = result.corruptions != 0; > + check->leaks_fixed = result.leaks_fixed; > + check->has_leaks_fixed = result.leaks != 0; > + check->highest_offset = result.highest_offset; > + check->has_highest_offset = result.highest_offset != 0; > + > + check->total_clusters = result.bfi.total_clusters; > + check->has_total_clusters = result.bfi.total_clusters != 0; > + check->allocated_clusters = result.bfi.allocated_clusters; > + check->has_allocated_clusters = result.bfi.allocated_clusters != 0; > + check->fragmented_clusters = result.bfi.fragmented_clusters; > + check->has_fragmented_clusters = result.bfi.fragmented_clusters != 0; Do the empty lines have any meaning? I think using them to group related fields together is a good idea, but I can't see the logic in this specific grouping. Also, can you align all = consistently to the same column? > + > + /* double checking the fixed image */ > + if (result.corruptions_fixed || result.leaks_fixed) { > + ret = bdrv_check(bs, &result, 0); > + > + if (ret < 0) { > + return ret; > + } > + > + check->corruptions = result.corruptions; > + check->has_corruptions = result.corruptions != 0; > + check->leaks = result.leaks; > + check->has_leaks = result.leaks != 0; > + } I think most fields should get the result from the second run, for example the number of allocated clusters could change when the image was repaired. You can leave filename/format and *_fixed where it is, and just move the initialisation of the other fields to below this if block. Then you also don't have to duplicate the lines for corruptions/leaks. Kevin