[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V3 2/2] qemu-img: Add json output option to the
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH V3 2/2] qemu-img: Add json output option to the info command. |
Date: |
Fri, 17 Aug 2012 11:36:58 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:13.0) Gecko/20120605 Thunderbird/13.0 |
Am 15.08.2012 20:48, schrieb Benoît Canet:
> This additionnal --machine=json option make qemu-img info output on
> stdout a JSON formated representation of the image informations.
>
> --machine=json was choosen instead of --format=json because the
> info command already have a -f parameter.
Which is not a problem. A machine doesn't care about shortcut options,
always specifying the long option is fine.
However, I wouldn't call that option --format (which would intuitively
relate to the image format) nor --machine (--machine=human looks really
weird), but --output=(json|human)
> #ifdef _WIN32
> @@ -84,6 +88,7 @@ static void help(void)
> " '-p' show progress of command (only certain commands)\n"
> " '-S' indicates the consecutive number of bytes that must
> contain only zeros\n"
> " for qemu-img to create a sparse image during conversion\n"
> + " '-m' or '--machine' takes the format in which the output must
> be done (json)\n"
You should mention both options, not only the non-default one.
> "\n"
> "Parameters to check subcommand:\n"
> " '-r' tries to repair any inconsistencies that are found during
> the check.\n"
> @@ -1102,21 +1107,86 @@ static void dump_snapshots(BlockDriverState *bs)
> g_free(sn_tab);
> }
>
> +static void collect_snapshots(BlockDriverState *bs , ImageInfo *image_info)
> +{
> + int i, sn_count;
> + QEMUSnapshotInfo *sn_tab = NULL;
> + SnapshotInfoList *sn_info_list, *cur_item = NULL;
> + sn_count = bdrv_snapshot_list(bs, &sn_tab);
> +
> + for (i = 0; i < sn_count; i++) {
> + image_info->has_snapshots = true;
> + sn_info_list = g_new0(SnapshotInfoList, 1);
> +
> + sn_info_list->value = g_new0(SnapshotInfo, 1);
> + sn_info_list->value->id = g_strdup(sn_tab[i].id_str);
> + sn_info_list->value->name = g_strdup(sn_tab[i].name);
> + sn_info_list->value->vm_state_size = sn_tab[i].vm_state_size;
> + sn_info_list->value->date_sec = sn_tab[i].date_sec;
> + sn_info_list->value->date_nsec = sn_tab[i].date_nsec;
> + sn_info_list->value->vm_clock_nsec = sn_tab[i].vm_clock_nsec;
Aligning the = to the same column wouldn't hurt.
> +
> + /* XXX: waiting for the qapi to support GSList */
Is this even planned? I would agree with qemu-queue.h structures
available from QAPI, but not GSList. Please change or remove the comment.
> + if (!cur_item) {
> + image_info->snapshots = cur_item = sn_info_list;
> + } else {
> + cur_item->next = sn_info_list;
> + cur_item = sn_info_list;
> + }
> +
> + }
> +
> + g_free(sn_tab);
> +}
> +
> +static void dump_json_image_info(ImageInfo *image_info)
> +{
> + Error *errp = NULL;
> + QString *str;
> + QmpOutputVisitor *ov = qmp_output_visitor_new();
> + QObject *obj;
> + visit_type_ImageInfo(qmp_output_get_visitor(ov),
> + &image_info, 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);
> +}
> +
> +#define PRINTH(human, args...) do { \
> + if (human) { \
> + printf(args); \
> + } } while (0);
Extra semicolon,
The existence of this macro doesn't look quite right at the first sight,
but let me check how it's used.
> +
> static int img_info(int argc, char **argv)
> {
> int c;
> - const char *filename, *fmt;
> - BlockDriverState *bs;
> + bool human = true;
> + const char *filename, *fmt, *machine;
> + BlockDriverState *bs, *backing_bs = NULL;
> char size_buf[128], dsize_buf[128];
> uint64_t total_sectors;
> int64_t allocated_size;
> char backing_filename[1024];
> char backing_filename2[1024];
> BlockDriverInfo bdi;
> + ImageInfo *image_info;
>
> fmt = NULL;
> + machine = NULL;
> for(;;) {
> - c = getopt(argc, argv, "f:h");
> + int option_index = 0;
> + static struct option long_options[] = {
> + {"help", no_argument, 0, 'h'},
> + {"format", required_argument, 0, 'f'},
> + {"machine", required_argument, 0, 'm'},
> + {0, 0, 0, 0}
> + };
> + c = getopt_long(argc, argv, "f:h",
> + long_options, &option_index);
> if (c == -1) {
> break;
> }
> @@ -1128,6 +1198,9 @@ static int img_info(int argc, char **argv)
> case 'f':
> fmt = optarg;
> break;
> + case 'm':
> + machine = optarg;
> + break;
> }
> }
> if (optind >= argc) {
> @@ -1135,8 +1208,14 @@ static int img_info(int argc, char **argv)
> }
> filename = argv[optind++];
>
> + image_info = g_new0(ImageInfo, 1);
> + if (machine && !strncmp(machine, "json", strlen("json"))) {
> + human = false;
> + }
Check against the valid values and exit with an error if it's neither
"json" nor "human" (or "text" or whatever you want to call it).
> +
> bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_NO_BACKING);
> if (!bs) {
> + g_free(image_info);
> return 1;
> }
> bdrv_get_geometry(bs, &total_sectors);
> @@ -1148,39 +1227,71 @@ static int img_info(int argc, char **argv)
> get_human_readable_size(dsize_buf, sizeof(dsize_buf),
> allocated_size);
> }
> - printf("image: %s\n"
> + PRINTH(human, "image: %s\n"
> "file format: %s\n"
> "virtual size: %s (%" PRId64 " bytes)\n"
> "disk size: %s\n",
> filename, bdrv_get_format_name(bs), size_buf,
> (total_sectors * 512),
> dsize_buf);
> + image_info->filename = g_strdup(filename);
> + image_info->format = g_strdup(bdrv_get_format_name(bs));
> + image_info->virtual_size = total_sectors * 512;
> + image_info->actual_size = allocated_size >= 0 ? allocated_size : 0;
> if (bdrv_is_encrypted(bs)) {
> - printf("encrypted: yes\n");
> + PRINTH(human, "encrypted: yes\n");
> + image_info->encrypted = true;
> + image_info->has_encrypted = true;
> }
> if (bdrv_get_info(bs, &bdi) >= 0) {
> + image_info->has_cluster_size = true;
> + image_info->has_dirty_flag = true;
> if (bdi.cluster_size != 0) {
> - printf("cluster_size: %d\n", bdi.cluster_size);
> + PRINTH(human, "cluster_size: %d\n", bdi.cluster_size);
> + image_info->cluster_size = bdi.cluster_size;
> }
> if (bdi.is_dirty) {
> - printf("cleanly shut down: no\n");
> + PRINTH(human, "cleanly shut down: no\n");
> + image_info->dirty_flag = true;
> + } else {
> + image_info->dirty_flag = false;
> }
> }
> bdrv_get_backing_filename(bs, backing_filename,
> sizeof(backing_filename));
> if (backing_filename[0] != '\0') {
> bdrv_get_full_backing_filename(bs, backing_filename2,
> sizeof(backing_filename2));
> - printf("backing file: %s", backing_filename);
> + backing_bs = bdrv_new_open(backing_filename2, fmt,
> + BDRV_O_FLAGS | BDRV_O_NO_BACKING);
> + image_info->backing_filename = g_strdup(backing_filename);
> + image_info->backing_filename_format =
> + g_strdup(bdrv_get_format_name(backing_bs));
> + bdrv_delete(backing_bs);
> + PRINTH(human, "backing file: %s", backing_filename);
> if (strcmp(backing_filename, backing_filename2) != 0) {
> - printf(" (actual path: %s)", backing_filename2);
> + PRINTH(human, " (actual path: %s)", backing_filename2);
> + }
> + if (human) {
> + putchar('\n');
> }
> - putchar('\n');
> + image_info->has_backing_filename = true;
> + image_info->has_backing_filename_format = true;
> }
> - dump_snapshots(bs);
> +
> + if (human) {
> + dump_snapshots(bs);
> + } else {
> + collect_snapshots(bs, image_info);
> + dump_json_image_info(image_info);
> + }
> +
> + qapi_free_ImageInfo(image_info);
> bdrv_delete(bs);
> return 0;
> }
I'm not sure if mixing both modes this way is a great idea. Maybe it's
better to have separate functions for either filling the ImageInfo
struct or human output, that are called only in the appropriate mode.
Kevin