From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:55293) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T8yNr-0002vP-95 for qemu-devel@nongnu.org; Tue, 04 Sep 2012 15:02:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T8yNl-0005Lb-Tm for qemu-devel@nongnu.org; Tue, 04 Sep 2012 15:01:59 -0400 Received: from paradis.irqsave.net ([109.190.18.76]:58391) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T8yNl-0005LG-Az for qemu-devel@nongnu.org; Tue, 04 Sep 2012 15:01:53 -0400 Date: Tue, 4 Sep 2012 21:01:29 +0200 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20120904190128.GA2190@irqsave.net> References: <1346051752-13551-1-git-send-email-benoit@irqsave.net> <1346051752-13551-3-git-send-email-benoit@irqsave.net> <5045F9FA.3090108@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <5045F9FA.3090108@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH V6 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: Kevin Wolf Cc: =?iso-8859-1?Q?Beno=EEt?= Canet , stefanha@linux.vnet.ibm.com, qemu-devel@nongnu.org, blauwirbel@gmail.com, pbonzini@redhat.com, eblake@redhat.com, xiawenc@linux.vnet.ibm.com Thanks for reviewing. I will correct the series tomorow. Beno=EEt Le Tuesday 04 Sep 2012 =E0 14:54:18 (+0200), Kevin Wolf a =E9crit : > Am 27.08.2012 09:15, schrieb Beno=EEt Canet: > > This option --output=3D[human|json] make qemu-img info output on > > human or JSON representation at the choice of the user. > >=20 > > example: > > { > > "snapshots": [ > > { > > "vm-clock-nsec": 637102488, > > "name": "vm-20120821145509", > > "date-sec": 1345553709, > > "date-nsec": 220289000, > > "vm-clock-sec": 20, > > "id": "1", > > "vm-state-size": 96522745 > > }, > > { > > "vm-clock-nsec": 28210866, > > "name": "vm-20120821154059", > > "date-sec": 1345556459, > > "date-nsec": 171392000, > > "vm-clock-sec": 46, > > "id": "2", > > "vm-state-size": 101208714 > > } > > ], > > "virtual-size": 1073741824, > > "filename": "snap.qcow2", > > "cluster-size": 65536, > > "format": "qcow2", > > "actual-size": 985587712, > > "dirty-flag": false > > } > >=20 > > Signed-off-by: Benoit Canet > > --- > > Makefile | 3 +- > > qemu-img.c | 259 ++++++++++++++++++++++++++++++++++++++++++++++++++= ---------- > > 2 files changed, 218 insertions(+), 44 deletions(-) > >=20 > > diff --git a/Makefile b/Makefile > > index ab82ef3..9ba064b 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -160,7 +160,8 @@ tools-obj-y =3D $(oslib-obj-y) $(trace-obj-y) qem= u-tool.o qemu-timer.o \ > > iohandler.o cutils.o iov.o async.o > > tools-obj-$(CONFIG_POSIX) +=3D compatfd.o > > =20 > > -qemu-img$(EXESUF): qemu-img.o $(tools-obj-y) $(block-obj-y) > > +qemu-img$(EXESUF): qemu-img.o $(tools-obj-y) $(block-obj-y) $(qapi-o= bj-y) \ > > + qapi-visit.o qapi-types.o > > qemu-nbd$(EXESUF): qemu-nbd.o $(tools-obj-y) $(block-obj-y) > > qemu-io$(EXESUF): qemu-io.o cmd.o $(tools-obj-y) $(block-obj-y) > > =20 > > diff --git a/qemu-img.c b/qemu-img.c > > index 80cfb9b..fe4a4fc 100644 > > --- a/qemu-img.c > > +++ b/qemu-img.c > > @@ -21,12 +21,16 @@ > > * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEA= LINGS IN > > * THE SOFTWARE. > > */ > > +#include "qapi-visit.h" > > +#include "qapi/qmp-output-visitor.h" > > +#include "qjson.h" > > #include "qemu-common.h" > > #include "qemu-option.h" > > #include "qemu-error.h" > > #include "osdep.h" > > #include "sysemu.h" > > #include "block_int.h" > > +#include > > #include > > =20 > > #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 mu= st contain only zeros\n" > > " for qemu-img to create a sparse image during conv= ersion\n" > > + " '--output' takes the format in which the output must b= e done (human or json)\n" > > "\n" > > "Parameters to check subcommand:\n" > > " '-r' tries to repair any inconsistencies that are foun= d during the check.\n" >=20 > qemu-img-cmds.hx and qemu-img.texi should be updated with documentation > of the new option as well. >=20 > > @@ -1102,21 +1107,196 @@ static void dump_snapshots(BlockDriverState = *bs) > > g_free(sn_tab); > > } > > =20 > > -static int img_info(int argc, char **argv) > > +static void collect_snapshots(BlockDriverState *bs , ImageInfo *info= ) > > +{ > > + int i, sn_count; > > + QEMUSnapshotInfo *sn_tab =3D NULL; > > + SnapshotInfoList *info_list, *cur_item =3D NULL; > > + sn_count =3D bdrv_snapshot_list(bs, &sn_tab); > > + > > + for (i =3D 0; i < sn_count; i++) { > > + info->has_snapshots =3D true; > > + info_list =3D g_new0(SnapshotInfoList, 1); > > + > > + info_list->value =3D g_new0(SnapshotInfo, 1); > > + info_list->value->id =3D g_strdup(sn_tab[i].id_st= r); > > + info_list->value->name =3D g_strdup(sn_tab[i].name)= ; > > + info_list->value->vm_state_size =3D sn_tab[i].vm_state_size; > > + info_list->value->date_sec =3D sn_tab[i].date_sec; > > + info_list->value->date_nsec =3D sn_tab[i].date_nsec; > > + info_list->value->vm_clock_sec =3D sn_tab[i].vm_clock_nsec = / 1000000000; > > + info_list->value->vm_clock_nsec =3D sn_tab[i].vm_clock_nsec = % 1000000000; > > + > > + /* XXX: waiting for the qapi to support GSList */ >=20 > No, GSList isn't typesafe and therefore to be avoided. If anything, the > support we want is for qemu-queue.h types. >=20 > > + if (!cur_item) { > > + info->snapshots =3D cur_item =3D info_list; > > + } else { > > + cur_item->next =3D info_list; > > + cur_item =3D info_list; > > + } > > + > > + } > > + > > + g_free(sn_tab); > > +} > > + > > +static void dump_json_image_info(ImageInfo *info) > > +{ > > + Error *errp =3D NULL; > > + QString *str; > > + QmpOutputVisitor *ov =3D qmp_output_visitor_new(); > > + QObject *obj; > > + visit_type_ImageInfo(qmp_output_get_visitor(ov), > > + &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); > > +} > > + > > +static void collect_backing_file_format(ImageInfo *info, char *filen= ame) > > +{ > > + BlockDriverState *bs =3D NULL; > > + bs =3D bdrv_new_open(filename, NULL, > > + BDRV_O_FLAGS | BDRV_O_NO_BACKING); > > + if (!bs) { > > + return; > > + } > > + info->backing_filename_format =3D > > + g_strdup(bdrv_get_format_name(bs)); > > + bdrv_delete(bs); > > + info->has_backing_filename_format =3D true; > > +} >=20 > This is new functionality and should be added in a separate patch. >=20 > It is also wrong because it uses NULL instead of the explicitly > specified format to open the image. >=20 > > + > > +static void collect_image_info(BlockDriverState *bs, > > + ImageInfo *info, > > + const char *filename, > > + const char *fmt) > > { > > - int c; > > - const char *filename, *fmt; > > - BlockDriverState *bs; > > - 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; > > + struct stat st; > > + > > + bdrv_get_geometry(bs, &total_sectors); > > + > > + info->filename =3D g_strdup(filename); > > + info->format =3D g_strdup(bdrv_get_format_name(bs)); > > + info->virtual_size =3D total_sectors * 512; > > + info->actual_size =3D bdrv_get_allocated_file_size(bs); > > + info->has_actual_size =3D info->actual_size >=3D 0; > > + if (bdrv_is_encrypted(bs)) { > > + info->encrypted =3D true; > > + info->has_encrypted =3D true; > > + } > > + if (bdrv_get_info(bs, &bdi) >=3D 0) { > > + if (bdi.cluster_size !=3D 0) { > > + info->cluster_size =3D bdi.cluster_size; > > + info->has_cluster_size =3D true; > > + } > > + if (bdi.is_dirty) { > > + info->dirty_flag =3D true; > > + } else { > > + info->dirty_flag =3D false; > > + } >=20 > Shorter written as: >=20 > info->dirty_flag =3D bdi.is_dirty; >=20 > > + info->has_dirty_flag =3D true; > > + } > > + bdrv_get_backing_filename(bs, backing_filename, sizeof(backing_f= ilename)); > > + if (backing_filename[0] !=3D '\0') { > > + info->backing_filename =3D g_strdup(backing_filename); > > + info->has_backing_filename =3D true; > > + bdrv_get_full_backing_filename(bs, backing_filename2, > > + sizeof(backing_filename2)); > > + > > + if (strcmp(backing_filename, backing_filename2) !=3D 0) { > > + info->full_backing_filename =3D > > + g_strdup(backing_filename2); > > + info->has_full_backing_filename =3D true; > > + } > > + > > + if (access(backing_filename2, R_OK) =3D=3D 0 && > > + !stat(backing_filename2, &st) && > > + S_ISREG(st.st_mode)) { >=20 > What are you doing here? This is most certainly wrong because it doesn'= t > consider protocols supported by qemu. >=20 > Also, I believe what you really want to expose is the backing file > format as specified in the image file (if any), i.e. bs->backing_format= . > Opening the backing isn't even necessary then. >=20 > > + collect_backing_file_format(info, backing_filename2)= ; > > + > > + } > > + } > > +} > > + > > +static void dump_human_image_info(ImageInfo *info) > > +{ > > + char size_buf[128], dsize_buf[128]; > > + if (!info->has_actual_size) { > > + snprintf(dsize_buf, sizeof(dsize_buf), "unavailable"); > > + } else { > > + get_human_readable_size(dsize_buf, sizeof(dsize_buf), > > + info->actual_size); > > + } > > + get_human_readable_size(size_buf, sizeof(size_buf), info->virtua= l_size); > > + printf("image: %s\n" > > + "file format: %s\n" > > + "virtual size: %s (%" PRId64 " bytes)\n" > > + "disk size: %s\n", > > + info->filename, info->format, size_buf, > > + info->virtual_size, > > + dsize_buf); > > + > > + if (info->has_encrypted && info->encrypted) { > > + printf("encrypted: yes\n"); > > + } > > + > > + if (info->has_cluster_size) { > > + printf("cluster_size: %" PRId64 "\n", info->cluster_size); > > + } > > + > > + if (info->has_dirty_flag && info->dirty_flag) { > > + printf("cleanly shut down: no\n"); > > + } > > + >=20 > > + if (info->has_backing_filename) { > > + printf("backing file: %s", info->backing_filename); > > + } > > + > > + if (info->has_full_backing_filename) { > > + printf(" (actual path: %s)", info->full_backing_filename); > > + } > > + > > + if (info->has_backing_filename) { > > + putchar('\n'); > > + } >=20 > The full filename is only present if the "normal" name is present > (otherwise the output would come out wrong anyway). So: >=20 > if (info->has_backing_filename) { > printf("backing file: %s", info->backing_filename); > if (info->has_full_backing_filename) { > printf(" (actual path: %s)", info->full_backing_filename); > } > putchar('\n'); > } >=20 > > +} > > + > > +enum {OPTION_OUTPUT =3D 256}; > > + > > +typedef enum Format { > > + FORMAT_JSON, > > + FORMAT_HUMAN, > > +} Format; >=20 > Hm, calling this Format is asking for naming conflicts. Maybe OutputFor= mat? >=20 > > + > > +static int img_info(int argc, char **argv) > > +{ > > + int c; > > + Format output_format; >=20 > Format output_format =3D FORMAT_HUMAN; >=20 > > + const char *filename, *fmt, *output; > > + BlockDriverState *bs; > > + ImageInfo *info; > > =20 > > fmt =3D NULL; > > + output =3D NULL; > > for(;;) { > > - c =3D getopt(argc, argv, "f:h"); > > + int option_index =3D 0; > > + static const struct option long_options[] =3D { > > + {"help", no_argument, 0, 'h'}, > > + {"format", required_argument, 0, 'f'}, > > + {"output", required_argument, 0, OPTION_OUTPUT}, > > + {0, 0, 0, 0} > > + }; > > + c =3D getopt_long(argc, argv, "f:h", > > + long_options, &option_index); > > if (c =3D=3D -1) { > > break; > > } > > @@ -1128,6 +1308,9 @@ static int img_info(int argc, char **argv) > > case 'f': > > fmt =3D optarg; > > break; > > + case OPTION_OUTPUT: > > + output =3D optarg; > > + break; > > } > > } > > if (optind >=3D argc) { > > @@ -1135,48 +1318,38 @@ static int img_info(int argc, char **argv) > > } > > filename =3D argv[optind++]; > > =20 > > - bs =3D bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_NO_BAC= KING); > > - if (!bs) { > > + if (output && !strcmp(output, "json")) { > > + output_format =3D FORMAT_JSON; > > + } else if (output && !strcmp(output, "human")) { > > + output_format =3D FORMAT_HUMAN; > > + } else if (output) { > > + fprintf(stderr, > > + "Error: --output must be used with human or json as argu= ment.\n"); >=20 > error_report()? >=20 > > return 1; > > - } > > - bdrv_get_geometry(bs, &total_sectors); > > - get_human_readable_size(size_buf, sizeof(size_buf), total_sector= s * 512); > > - allocated_size =3D bdrv_get_allocated_file_size(bs); > > - if (allocated_size < 0) { > > - snprintf(dsize_buf, sizeof(dsize_buf), "unavailable"); > > } else { > > - get_human_readable_size(dsize_buf, sizeof(dsize_buf), > > - allocated_size); > > + output_format =3D FORMAT_HUMAN; > > } >=20 > When you add the default above, you don't need the else branch here. >=20 > > - printf("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); > > - if (bdrv_is_encrypted(bs)) { > > - printf("encrypted: yes\n"); > > + > > + info =3D g_new0(ImageInfo, 1); > > + bs =3D bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_NO_BAC= KING); > > + if (!bs) { > > + g_free(info); > > + return 1; > > } > > - if (bdrv_get_info(bs, &bdi) >=3D 0) { > > - if (bdi.cluster_size !=3D 0) { > > - printf("cluster_size: %d\n", bdi.cluster_size); > > - } > > - if (bdi.is_dirty) { > > - printf("cleanly shut down: no\n"); > > - } > > + > > + collect_image_info(bs, info, filename, fmt); > > + > > + if (output_format =3D=3D FORMAT_HUMAN) { > > + dump_human_image_info(info); > > + dump_snapshots(bs); > > } > > - bdrv_get_backing_filename(bs, backing_filename, sizeof(backing_f= ilename)); > > - if (backing_filename[0] !=3D '\0') { > > - bdrv_get_full_backing_filename(bs, backing_filename2, > > - sizeof(backing_filename2)); > > - printf("backing file: %s", backing_filename); > > - if (strcmp(backing_filename, backing_filename2) !=3D 0) { > > - printf(" (actual path: %s)", backing_filename2); > > - } > > - putchar('\n'); > > + > > + if (output_format =3D=3D FORMAT_JSON) { > > + collect_snapshots(bs, info); > > + dump_json_image_info(info); > > } >=20 > Use a switch instead of two independent ifs? >=20 > > - dump_snapshots(bs); > > + > > + qapi_free_ImageInfo(info); > > bdrv_delete(bs); > > return 0; > > } > >=20 >=20 > Kevin