From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:46515) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T2IzU-00016w-4e for qemu-devel@nongnu.org; Fri, 17 Aug 2012 05:37:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T2IzP-000278-EW for qemu-devel@nongnu.org; Fri, 17 Aug 2012 05:37:16 -0400 Received: from mx1.redhat.com ([209.132.183.28]:15669) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T2IzP-00025v-6J for qemu-devel@nongnu.org; Fri, 17 Aug 2012 05:37:11 -0400 Message-ID: <502E10BA.2010402@redhat.com> Date: Fri, 17 Aug 2012 11:36:58 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <1345056523-10249-1-git-send-email-benoit@irqsave.net> <1345056523-10249-3-git-send-email-benoit@irqsave.net> In-Reply-To: <1345056523-10249-3-git-send-email-benoit@irqsave.net> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH V3 2/2] qemu-img: Add json output option to the info command. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?ISO-8859-15?Q?Beno=EEt_Canet?= Cc: aliguori@us.ibm.com, stefanha@linux.vnet.ibm.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, pbonzini@redhat.com, eblake@redhat.com, xiawenc@linux.vnet.ibm.com, =?ISO-8859-15?Q?Beno=EEt_Canet?= Am 15.08.2012 20:48, schrieb Beno=EEt Canet: > This additionnal --machine=3Djson option make qemu-img info output on > stdout a JSON formated representation of the image informations. >=20 > --machine=3Djson was choosen instead of --format=3Djson 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=3Dhuman looks really weird), but --output=3D(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 conver= sion\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); > } > =20 > +static void collect_snapshots(BlockDriverState *bs , ImageInfo *image_= info) > +{ > + int i, sn_count; > + QEMUSnapshotInfo *sn_tab =3D NULL; > + SnapshotInfoList *sn_info_list, *cur_item =3D NULL; > + sn_count =3D bdrv_snapshot_list(bs, &sn_tab); > + > + for (i =3D 0; i < sn_count; i++) { > + image_info->has_snapshots =3D true; > + sn_info_list =3D g_new0(SnapshotInfoList, 1); > + > + sn_info_list->value =3D g_new0(SnapshotInfo, 1); > + sn_info_list->value->id =3D g_strdup(sn_tab[i].id_str); > + sn_info_list->value->name =3D g_strdup(sn_tab[i].name); > + sn_info_list->value->vm_state_size =3D sn_tab[i].vm_state_size= ; > + sn_info_list->value->date_sec =3D sn_tab[i].date_sec; > + sn_info_list->value->date_nsec =3D sn_tab[i].date_nsec; > + sn_info_list->value->vm_clock_nsec =3D sn_tab[i].vm_clock_nsec= ; Aligning the =3D 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 =3D cur_item =3D sn_info_list; > + } else { > + cur_item->next =3D sn_info_list; > + cur_item =3D sn_info_list; > + } > + > + } > + > + g_free(sn_tab); > +} > + > +static void dump_json_image_info(ImageInfo *image_info) > +{ > + Error *errp =3D NULL; > + QString *str; > + QmpOutputVisitor *ov =3D qmp_output_visitor_new(); > + QObject *obj; > + visit_type_ImageInfo(qmp_output_get_visitor(ov), > + &image_info, NULL, &errp); > + obj =3D qmp_output_get_qobject(ov); > + str =3D qobject_to_json_pretty(obj); > + assert(str !=3D 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 =3D true; > + const char *filename, *fmt, *machine; > + BlockDriverState *bs, *backing_bs =3D 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; > =20 > fmt =3D NULL; > + machine =3D NULL; > for(;;) { > - c =3D getopt(argc, argv, "f:h"); > + int option_index =3D 0; > + static struct option long_options[] =3D { > + {"help", no_argument, 0, 'h'}, > + {"format", required_argument, 0, 'f'}, > + {"machine", required_argument, 0, 'm'}, > + {0, 0, 0, 0} > + }; > + c =3D getopt_long(argc, argv, "f:h", > + long_options, &option_index); > if (c =3D=3D -1) { > break; > } > @@ -1128,6 +1198,9 @@ static int img_info(int argc, char **argv) > case 'f': > fmt =3D optarg; > break; > + case 'm': > + machine =3D optarg; > + break; > } > } > if (optind >=3D argc) { > @@ -1135,8 +1208,14 @@ static int img_info(int argc, char **argv) > } > filename =3D argv[optind++]; > =20 > + image_info =3D g_new0(ImageInfo, 1); > + if (machine && !strncmp(machine, "json", strlen("json"))) { > + human =3D 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 =3D bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_NO_BACKI= NG); > 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 =3D g_strdup(filename); > + image_info->format =3D g_strdup(bdrv_get_format_name(bs)); > + image_info->virtual_size =3D total_sectors * 512; > + image_info->actual_size =3D allocated_size >=3D 0 ? allocated_size= : 0; > if (bdrv_is_encrypted(bs)) { > - printf("encrypted: yes\n"); > + PRINTH(human, "encrypted: yes\n"); > + image_info->encrypted =3D true; > + image_info->has_encrypted =3D true; > } > if (bdrv_get_info(bs, &bdi) >=3D 0) { > + image_info->has_cluster_size =3D true; > + image_info->has_dirty_flag =3D true; > if (bdi.cluster_size !=3D 0) { > - printf("cluster_size: %d\n", bdi.cluster_size); > + PRINTH(human, "cluster_size: %d\n", bdi.cluster_size); > + image_info->cluster_size =3D bdi.cluster_size; > } > if (bdi.is_dirty) { > - printf("cleanly shut down: no\n"); > + PRINTH(human, "cleanly shut down: no\n"); > + image_info->dirty_flag =3D true; > + } else { > + image_info->dirty_flag =3D false; > } > } > bdrv_get_backing_filename(bs, backing_filename, sizeof(backing_fil= ename)); > if (backing_filename[0] !=3D '\0') { > bdrv_get_full_backing_filename(bs, backing_filename2, > sizeof(backing_filename2)); > - printf("backing file: %s", backing_filename); > + backing_bs =3D bdrv_new_open(backing_filename2, fmt, > + BDRV_O_FLAGS | BDRV_O_NO_BACKING); > + image_info->backing_filename =3D g_strdup(backing_filename); > + image_info->backing_filename_format =3D > + 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) !=3D 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 =3D true; > + image_info->has_backing_filename_format =3D 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