Patchwork [2/2] qemu-img: add json output option to the check command

login
register
mail settings
Submitter Federico Simoncelli
Date Dec. 10, 2012, 5:01 p.m.
Message ID <1355158885-21285-2-git-send-email-fsimonce@redhat.com>
Download mbox | patch
Permalink /patch/204988/
State New
Headers show

Comments

Federico Simoncelli - Dec. 10, 2012, 5:01 p.m.
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 <fsimonce@redhat.com>
---
 qapi-schema.json |   38 +++++++++
 qemu-img-cmds.hx |    4 +-
 qemu-img.c       |  246 +++++++++++++++++++++++++++++++++++++++---------------
 qemu-img.texi    |    5 +-
 4 files changed, 221 insertions(+), 72 deletions(-)
Kevin Wolf - Dec. 13, 2012, 12:38 p.m.
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 <fsimonce@redhat.com>
> ---
>  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

Patch

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
+#
+# @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
+#
+# @total-clusters: #optional total number of clusters
+#
+# @allocated-clusters: #optional total number of allocated clusters
+#
+# @fragmented-clusters: #optional total number of fragmented clusters
+#
+# 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);
+    }
+
+    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;
+
+    /* 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;
+    }
+
+    return 0;
+}
+
 /*
  * Checks an image for consistency. Exit codes:
  *
@@ -381,15 +507,26 @@  out:
 static int img_check(int argc, char **argv)
 {
     int c, ret;
-    const char *filename, *fmt;
+    OutputFormat output_format = OFORMAT_HUMAN;
+    const char *filename, *fmt, *output;
     BlockDriverState *bs;
-    BdrvCheckResult result;
     int fix = 0;
     int flags = BDRV_O_FLAGS | BDRV_O_CHECK;
+    ImageCheck *check;
 
     fmt = NULL;
+    output = NULL;
     for(;;) {
-        c = getopt(argc, argv, "f:hr:");
+        int option_index = 0;
+        static const struct option long_options[] = {
+            {"help", no_argument, 0, 'h'},
+            {"format", required_argument, 0, 'f'},
+            {"repair", no_argument, 0, 'r'},
+            {"output", required_argument, 0, OPTION_OUTPUT},
+            {0, 0, 0, 0}
+        };
+        c = getopt_long(argc, argv, "f:hr:",
+                        long_options, &option_index);
         if (c == -1) {
             break;
         }
@@ -412,84 +549,67 @@  static int img_check(int argc, char **argv)
                 help();
             }
             break;
+        case OPTION_OUTPUT:
+            output = optarg;
+            break;
         }
     }
+
     if (optind >= argc) {
         help();
     }
+
     filename = argv[optind++];
 
-    bs = bdrv_new_open(filename, fmt, flags, true);
-    if (!bs) {
+    if (output && !strcmp(output, "json")) {
+        output_format = OFORMAT_JSON;
+    } else if (output && !strcmp(output, "human")) {
+        output_format = OFORMAT_HUMAN;
+    } else if (output) {
+        error_report("--output must be used with human or json as argument.");
         return 1;
     }
-    ret = bdrv_check(bs, &result, fix);
 
-    if (ret == -ENOTSUP) {
-        error_report("This image format does not support checks");
-        bdrv_delete(bs);
+    bs = bdrv_new_open(filename, fmt, flags, true);
+    if (!bs) {
         return 1;
     }
 
-    if (result.corruptions_fixed || result.leaks_fixed) {
-        printf("The following inconsistencies were found and repaired:\n\n"
-               "    %d leaked clusters\n"
-               "    %d corruptions\n\n"
-               "Double checking the fixed image now...\n",
-               result.leaks_fixed,
-               result.corruptions_fixed);
-        ret = bdrv_check(bs, &result, 0);
-    }
+    check = g_new0(ImageCheck, 1);
+    ret = collect_image_check(bs, check, filename, fmt, fix);
 
-    if (!(result.corruptions || result.leaks || result.check_errors)) {
-        printf("No errors were found on the image.\n");
-    } else {
-        if (result.corruptions) {
-            printf("\n%d errors were found on the image.\n"
-                "Data may be corrupted, or further writes to the image "
-                "may corrupt it.\n",
-                result.corruptions);
-        }
-
-        if (result.leaks) {
-            printf("\n%d leaked clusters were found on the image.\n"
-                "This means waste of disk space, but no harm to data.\n",
-                result.leaks);
-        }
+    if (ret == -ENOTSUP) {
+        ret = 1;
+        goto fail;
+    }
 
-        if (result.check_errors) {
-            printf("\n%d internal errors have occurred during the check.\n",
-                result.check_errors);
-        }
+    switch (output_format) {
+    case OFORMAT_HUMAN:
+        dump_human_image_check(check);
+        break;
+    case OFORMAT_JSON:
+        dump_json_image_check(check);
+        break;
     }
 
-    if (result.bfi.total_clusters != 0 && result.bfi.allocated_clusters != 0) {
-        printf("%" PRId64 "/%" PRId64 "= %0.2f%% allocated, %0.2f%% fragmented\n",
-        result.bfi.allocated_clusters, result.bfi.total_clusters,
-        result.bfi.allocated_clusters * 100.0 / result.bfi.total_clusters,
-        result.bfi.fragmented_clusters * 100.0 / result.bfi.allocated_clusters);
+    if (ret || check->check_errors) {
+        ret = 1;
+        goto fail;
     }
 
-    if (result.highest_offset > 0) {
-        printf("Highest offset in use: %" PRId64 "\n", result.highest_offset);
+    if (check->corruptions) {
+        ret = 2;
+    } else if (check->leaks) {
+        ret = 3;
+    } else {
+        ret = 0;
     }
 
+fail:
+    qapi_free_ImageCheck(check);
     bdrv_delete(bs);
 
-    if (ret < 0 || result.check_errors) {
-        printf("\nAn error has occurred during the check: %s\n"
-            "The check is not complete and may have missed error.\n",
-            strerror(-ret));
-        return 1;
-    }
-
-    if (result.corruptions) {
-        return 2;
-    } else if (result.leaks) {
-        return 3;
-    } else {
-        return 0;
-    }
+    return ret;
 }
 
 static int img_commit(int argc, char **argv)
@@ -1391,16 +1511,6 @@  err:
     return NULL;
 }
 
-enum {
-    OPTION_OUTPUT = 256,
-    OPTION_BACKING_CHAIN = 257,
-};
-
-typedef enum OutputFormat {
-    OFORMAT_JSON,
-    OFORMAT_HUMAN,
-} OutputFormat;
-
 static int img_info(int argc, char **argv)
 {
     int c;
diff --git a/qemu-img.texi b/qemu-img.texi
index 00fca8d..1a6c9e3 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -84,9 +84,10 @@  lists all snapshots in the given image
 Command description:
 
 @table @option
-@item check [-f @var{fmt}] [-r [leaks | all]] @var{filename}
+@item check [-f @var{fmt}] [--output=@var{ofmt}] [-r [leaks | all]] @var{filename}
 
-Perform a consistency check on the disk image @var{filename}.
+Perform a consistency check on the disk image @var{filename}. The command can
+output in the format @var{ofmt} which is either @code{human} or @code{json}.
 
 If @code{-r} is specified, qemu-img tries to repair any inconsistencies found
 during the check. @code{-r leaks} repairs only cluster leaks, whereas