Re: [Qemu-devel] [PATCH 2/2] qemu-img: add json output option to the check command

[Kevin Wolf]

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 <address@hidden>
> ---
>  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
> address@hidden check [-f @var{fmt}] [-r [leaks | all]] @var{filename}
> address@hidden check [-f @var{fmt}] address@hidden [-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