[Qemu-devel] [PATCH 0/3] Add JSON output to qemu-img info

[Qemu-devel] [PATCH 0/3] Add JSON output to qemu-img info

[Benoît Canet]

This patchset add a JSON output mode to the qemu-img info command.
It's a rewrite from scratch of the original patchset by Wenchao Xia
following Anthony Liguori advices on JSON formating.

To use it simply add --machine=json to the command line.

Benoît Canet (3):
  qapi: Add SnapshotInfo.
  qapi: Add ImageInfo.
  qemu-img: Add json output option to the info command.

 Makefile         |    3 +-
 qapi-schema.json |   57 ++++++++++++++++++++++++
 qemu-img.c       |  126 +++++++++++++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 175 insertions(+), 11 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCH 1/3] qapi: Add SnapshotInfo.

[Benoît Canet]

Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
 qapi-schema.json |   24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/qapi-schema.json b/qapi-schema.json
index a92adb1..8d4df19 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -126,6 +126,30 @@
             'running', 'save-vm', 'shutdown', 'suspended', 'watchdog' ] }
 
 ##
+# @SnapshotInfo
+#
+# @id: unique snapshot id
+#
+# @name: user choosen name
+#
+# @vm-state-size: size of the VM state
+#
+# @date-sec: UTC date of the snapshot
+#
+# @date-nsec: date in nano seconds
+#
+# @vm-clock-sec: VM clock relative to boot
+#
+# Since: 1.3
+#
+##
+
+{ 'type': 'SnapshotInfo',
+  'data': { 'id': 'str', 'name': 'str', 'vm-state-size': 'int',
+            'date-sec': 'int', 'date-nsec': 'int',
+            'vm-clock-sec': 'int' } }
+
+##
 # @StatusInfo:
 #
 # Information about VCPU run state
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 1/3] qapi: Add SnapshotInfo.

[Luiz Capitulino]

On Wed, 15 Aug 2012 16:54:24 +0200
"Benoît Canet" <benoit.canet@gmail.com> wrote:

> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  qapi-schema.json |   24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index a92adb1..8d4df19 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -126,6 +126,30 @@
>              'running', 'save-vm', 'shutdown', 'suspended', 'watchdog' ] }
>  
>  ##
> +# @SnapshotInfo
> +#
> +# @id: unique snapshot id
> +#
> +# @name: user choosen name
> +#
> +# @vm-state-size: size of the VM state
> +#
> +# @date-sec: UTC date of the snapshot
> +#
> +# @date-nsec: date in nano seconds
> +#
> +# @vm-clock-sec: VM clock relative to boot
> +#
> +# Since: 1.3
> +#
> +##
> +
> +{ 'type': 'SnapshotInfo',
> +  'data': { 'id': 'str', 'name': 'str', 'vm-state-size': 'int',
> +            'date-sec': 'int', 'date-nsec': 'int',
> +            'vm-clock-sec': 'int' } }

Can I assume that this matches with what should be returned by the (hopefully
not so distant) future query-snapshots command?

> +
> +##
>  # @StatusInfo:
>  #
>  # Information about VCPU run state

Re: [Qemu-devel] [PATCH 1/3] qapi: Add SnapshotInfo.

[Benoît Canet]

Le Wednesday 15 Aug 2012 à 14:58:45 (-0300), Luiz Capitulino a écrit :
> On Wed, 15 Aug 2012 16:54:24 +0200
> "Benoît Canet" <benoit.canet@gmail.com> wrote:
> 
> > Signed-off-by: Benoit Canet <benoit@irqsave.net>
> > ---
> >  qapi-schema.json |   24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index a92adb1..8d4df19 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -126,6 +126,30 @@
> >              'running', 'save-vm', 'shutdown', 'suspended', 'watchdog' ] }
> >  
> >  ##
> > +# @SnapshotInfo
> > +#
> > +# @id: unique snapshot id
> > +#
> > +# @name: user choosen name
> > +#
> > +# @vm-state-size: size of the VM state
> > +#
> > +# @date-sec: UTC date of the snapshot
> > +#
> > +# @date-nsec: date in nano seconds
> > +#
> > +# @vm-clock-sec: VM clock relative to boot
> > +#
> > +# Since: 1.3
> > +#
> > +##
> > +
> > +{ 'type': 'SnapshotInfo',
> > +  'data': { 'id': 'str', 'name': 'str', 'vm-state-size': 'int',
> > +            'date-sec': 'int', 'date-nsec': 'int',
> > +            'vm-clock-sec': 'int' } }
> 
> Can I assume that this matches with what should be returned by the (hopefully
> not so distant) future query-snapshots command?
These are the field of the struture in block.h.
yet I don't know if it answer correctly to the question :)

> 
> > +
> > +##
> >  # @StatusInfo:
> >  #
> >  # Information about VCPU run state
> 
> 

Re: [Qemu-devel] [PATCH 1/3] qapi: Add SnapshotInfo.

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1100 bytes --]

On 08/15/2012 08:54 AM, Benoît Canet wrote:
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  qapi-schema.json |   24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index a92adb1..8d4df19 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -126,6 +126,30 @@
>              'running', 'save-vm', 'shutdown', 'suspended', 'watchdog' ] }
>  
>  ##
> +# @SnapshotInfo
> +#
> +# @id: unique snapshot id
> +#
> +# @name: user choosen name
> +#
> +# @vm-state-size: size of the VM state
> +#
> +# @date-sec: UTC date of the snapshot
> +#
> +# @date-nsec: date in nano seconds

How come we track a subsecond creation time,

> +#
> +# @vm-clock-sec: VM clock relative to boot

but not a subsecond relative time for the VM clock?  Is that additional
precision available for querying?

I have no problem if you want to squash patches 1 and 2 together into a
single commit.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [PATCH 1/3] qapi: Add SnapshotInfo.

[Benoît Canet]

Le Wednesday 15 Aug 2012 à 12:04:56 (-0600), Eric Blake a écrit :
> On 08/15/2012 08:54 AM, Benoît Canet wrote:
> > Signed-off-by: Benoit Canet <benoit@irqsave.net>
> > ---
> >  qapi-schema.json |   24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index a92adb1..8d4df19 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -126,6 +126,30 @@
> >              'running', 'save-vm', 'shutdown', 'suspended', 'watchdog' ] }
> >  
> >  ##
> > +# @SnapshotInfo
> > +#
> > +# @id: unique snapshot id
> > +#
> > +# @name: user choosen name
> > +#
> > +# @vm-state-size: size of the VM state
> > +#
> > +# @date-sec: UTC date of the snapshot
> > +#
> > +# @date-nsec: date in nano seconds
> 
I took all the availables fields:

typedef struct QEMUSnapshotInfo {
    char id_str[128]; /* unique snapshot id */
    /* the following fields are informative. They are not needed for
       the consistency of the snapshot */
    char name[256]; /* user chosen name */
    uint64_t vm_state_size; /* VM state info size */
    uint32_t date_sec; /* UTC date of the snapshot */
    uint32_t date_nsec;
    uint64_t vm_clock_nsec; /* VM clock relative to boot */
} QEMUSnapshotInfo;


> How come we track a subsecond creation time,
> 
> > +#
> > +# @vm-clock-sec: VM clock relative to boot
> 
> but not a subsecond relative time for the VM clock?  Is that additional
> precision available for querying?

This one is vm-clock-nsec I made a typo


> 
> I have no problem if you want to squash patches 1 and 2 together into a
> single commit.
ok

Benoît

> 
> -- 
> Eric Blake   eblake@redhat.com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

[Qemu-devel] [PATCH 2/3] qapi: Add ImageInfo.

[Benoît Canet]

Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
 qapi-schema.json |   33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/qapi-schema.json b/qapi-schema.json
index 8d4df19..488da90 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -150,6 +150,39 @@
             'vm-clock-sec': 'int' } }
 
 ##
+# @ImageInfo:
+#
+# Information about a QEMU image file
+#
+# @filename: name of the image file
+#
+# @format: format of the image file
+#
+# @dirty-flag: true if image is not cleanly closed
+#
+# @cluster-size: size of a cluster in bytes
+#
+# @encrypted: true if the image is encrypted
+#
+# @actual-size: actual size on disk in bytes of the image
+#
+# @virtual-size: maximum capacity in bytes of the image
+#
+# @backing-filename: name of the backing file
+#
+# @snapshots: list of VM snapshots
+#
+# Since: 1.3
+#
+##
+
+{ 'type': 'ImageInfo',
+  'data': {'filename': 'str', 'format': 'str', 'dirty-flag': 'bool',
+           'cluster-size': 'int', 'encrypted': 'bool',
+           'actual-size': 'int', 'virtual-size': 'int',
+           'backing-filename': 'str', 'snapshots': ['SnapshotInfo'] } }
+
+##
 # @StatusInfo:
 #
 # Information about VCPU run state
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 2/3] qapi: Add ImageInfo.

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1676 bytes --]

On 08/15/2012 08:54 AM, Benoît Canet wrote:
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  qapi-schema.json |   33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 8d4df19..488da90 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -150,6 +150,39 @@
>              'vm-clock-sec': 'int' } }
>  
>  ##
> +# @ImageInfo:
> +#
> +# Information about a QEMU image file
> +#
> +# @filename: name of the image file
> +#
> +# @format: format of the image file
> +#
> +# @dirty-flag: true if image is not cleanly closed
> +#
> +# @cluster-size: size of a cluster in bytes
> +#
> +# @encrypted: true if the image is encrypted
> +#
> +# @actual-size: actual size on disk in bytes of the image
> +#
> +# @virtual-size: maximum capacity in bytes of the image
> +#
> +# @backing-filename: name of the backing file

Should this be #optional, in the case where there is no backing file?

Missing @backing-filename-format, when that is recorded (as long as
we're going for machine-parseable, print everything useful, even if
qemu-img info doesn't print it right now).  Definitely #optional, since
it won't be present when @backing-filename is missing, and even when a
backing file name is present, the format might not have been recorded.

> +#
> +# @snapshots: list of VM snapshots

Should this be #optional, in the case where there are no snapshots?

> +#
> +# Since: 1.3

So no way it will make 1.2 hard freeze today :)

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [PATCH 2/3] qapi: Add ImageInfo.

[Benoît Canet]

> > +#
> > +# Since: 1.3
> 
> So no way it will make 1.2 hard freeze today :)

I would like to.Just tell me if it would be possible :)

> 
> -- 
> Eric Blake   eblake@redhat.com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

Re: [Qemu-devel] [PATCH 2/3] qapi: Add ImageInfo.

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2279 bytes --]

On 08/15/2012 10:17 AM, Eric Blake wrote:
> On 08/15/2012 08:54 AM, Benoît Canet wrote:
>> Signed-off-by: Benoit Canet <benoit@irqsave.net>
>> ---
>>  qapi-schema.json |   33 +++++++++++++++++++++++++++++++++
>>  1 file changed, 33 insertions(+)

>> +# @ImageInfo:
>> +#
>> +# Information about a QEMU image file
>> +#
>> +# @filename: name of the image file
>> +#
>> +# @format: format of the image file

These always exist.

>> +#
>> +# @dirty-flag: true if image is not cleanly closed

This exists for QED and qcow2v3, but not for raw or qcow2 without v3
extensions, so probably worth marking #optional

>> +#
>> +# @cluster-size: size of a cluster in bytes

This exists for qcow2, but not for raw, so probably worth marking #optional

>> +#
>> +# @encrypted: true if the image is encrypted

Again, probably #optional

>> +#
>> +# @actual-size: actual size on disk in bytes of the image

Always exists.  But is this the size of the contents seen by the guest,
or the size of the metadata seen by the host?  Both numbers are useful.

>> +#
>> +# @virtual-size: maximum capacity in bytes of the image

Tough call.  For qcow2, this would be read from the metadata.  For raw,
do we list the current size of the file, or do we list the size of free
space on the file system containing the file, or do we omit it for raw?

>> +#
>> +# @backing-filename: name of the backing file
> 
> Should this be #optional, in the case where there is no backing file?
> 
> Missing @backing-filename-format, when that is recorded (as long as
> we're going for machine-parseable, print everything useful, even if
> qemu-img info doesn't print it right now).  Definitely #optional, since
> it won't be present when @backing-filename is missing, and even when a
> backing file name is present, the format might not have been recorded.
> 
>> +#
>> +# @snapshots: list of VM snapshots
> 
> Should this be #optional, in the case where there are no snapshots?
> 
>> +#
>> +# Since: 1.3
> 
> So no way it will make 1.2 hard freeze today :)

Actually, I'd like to see this in 1.2, if we can muster up enough other
reviewers.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

[Qemu-devel] [PATCH 3/3] qemu-img: Add json output option to the info command.

[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.

example:
{
    "snapshots": [
        {
            "name": "truc",
            "date-sec": 1345034924,
            "date-nsec": 870405000,
            "vm-clock-sec": 4647590161,
            "id": "1",
            "vm-state-size": 80805256
        },
        {
            "name": "onx",
            "date-sec": 1345034927,
            "date-nsec": 914633000,
            "vm-clock-sec": 7065282574,
            "id": "2",
            "vm-state-size": 75927370
        },
        {
            "name": "list",
            "date-sec": 1345034939,
            "date-nsec": 39119000,
            "vm-clock-sec": 10108695046,
            "id": "3",
            "vm-state-size": 75890567
        }
    ],
    "virtual-size": 1073741824,
    "filename": "snapshot.img",
    "cluster-size": 65536,
    "format": "qcow2",
    "actual-size": 242614272,
    "encrypted": false,
    "backing-filename": "truc5.raw",
    "dirty-flag": false
}

Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
 Makefile   |    3 +-
 qemu-img.c |  126 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 118 insertions(+), 11 deletions(-)

diff --git a/Makefile b/Makefile
index ab82ef3..9ba064b 100644
--- a/Makefile
+++ b/Makefile
@@ -160,7 +160,8 @@ tools-obj-y = $(oslib-obj-y) $(trace-obj-y) qemu-tool.o qemu-timer.o \
 	iohandler.o cutils.o iov.o async.o
 tools-obj-$(CONFIG_POSIX) += compatfd.o
 
-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-obj-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)
 
diff --git a/qemu-img.c b/qemu-img.c
index 80cfb9b..1a80414 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 DEALINGS 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 <getopt.h>
 #include <stdio.h>
 
 #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"
            "\n"
            "Parameters to check subcommand:\n"
            "  '-r' tries to repair any inconsistencies that are found during the check.\n"
@@ -1102,10 +1107,66 @@ 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;
+    SnapshotInfoList *sn_info_list, *cur_item = NULL, *head = NULL;
+    sn_count = bdrv_snapshot_list(bs, &sn_tab);
+
+    for (i = 0; i < sn_count; i++) {
+        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_sec = sn_tab[i].vm_clock_nsec;
+
+        /* XXX: waiting for the qapi to support GSList */
+        if (!cur_item) {
+            head = cur_item = sn_info_list;
+        } else {
+            cur_item->next = sn_info_list;
+            cur_item = sn_info_list;
+        }
+
+    }
+
+    g_free(sn_tab);
+    image_info->snapshots = head;
+}
+
+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);
+    qstring_append_chr(str, '\n');
+    printf("%s", 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);
+
 static int img_info(int argc, char **argv)
 {
     int c;
-    const char *filename, *fmt;
+    bool human = true;
+    const char *filename, *fmt, *machine;
     BlockDriverState *bs;
     char size_buf[128], dsize_buf[128];
     uint64_t total_sectors;
@@ -1113,10 +1174,20 @@ static int img_info(int argc, char **argv)
     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 +1199,9 @@ static int img_info(int argc, char **argv)
         case 'f':
             fmt = optarg;
             break;
+        case 'm':
+            machine = optarg;
+            break;
         }
     }
     if (optind >= argc) {
@@ -1135,6 +1209,11 @@ 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;
+    }
+
     bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_NO_BACKING);
     if (!bs) {
         return 1;
@@ -1148,39 +1227,66 @@ 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;
+    } else {
+        image_info->encrypted = false;
     }
     if (bdrv_get_info(bs, &bdi) >= 0) {
         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') {
+        image_info->backing_filename = g_strdup(backing_filename);
         bdrv_get_full_backing_filename(bs, backing_filename2,
                                        sizeof(backing_filename2));
-        printf("backing file: %s", backing_filename);
+        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');
     }
-    dump_snapshots(bs);
+
+    if (human) {
+        dump_snapshots(bs);
+    } else {
+        collect_snapshots(bs, image_info);
+    }
+
+    if (!human) {
+        dump_json_image_info(image_info);
+    }
+
+    qapi_free_ImageInfo(image_info);
     bdrv_delete(bs);
     return 0;
 }
 
+#undef PRINTH
+
 #define SNAPSHOT_LIST   1
 #define SNAPSHOT_CREATE 2
 #define SNAPSHOT_APPLY  3
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 3/3] qemu-img: Add json output option to the info command.

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1324 bytes --]

On 08/15/2012 08:54 AM, Benoît Canet wrote:
> 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.

Do we want a counterpart '--machine=text' or '--machine=human' to
explicitly specify current output format?

> @@ -1113,10 +1174,20 @@ static int img_info(int argc, char **argv)
>      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);

getopt_long is not in POSIX.  Are you sure this will compile on all
target platforms?

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [PATCH 3/3] qemu-img: Add json output option to the info command.

[Benoît Canet]

Le Wednesday 15 Aug 2012 à 10:23:32 (-0600), Eric Blake a écrit :
> On 08/15/2012 08:54 AM, Benoît Canet wrote:
> > 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.
> 
> Do we want a counterpart '--machine=text' or '--machine=human' to
> explicitly specify current output format?
> 
> > @@ -1113,10 +1174,20 @@ static int img_info(int argc, char **argv)
> >      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);
> 
> getopt_long is not in POSIX.  Are you sure this will compile on all
> target platforms?

benoit@Laure:~/qemu$ git grep getopt_long
fsdev/virtfs-proxy-helper.c:        c = getopt_long(argc, argv, "p:nh?f:s:u:g:", helper_opts,
qemu-ga.c:    while ((ch = getopt_long(argc, argv, sopt, lopt, &opt_ind)) != -1) {
qemu-io.c:    while ((c = getopt_long(argc, argv, sopt, lopt, &opt_index)) != -1) {
qemu-nbd.c:    while ((ch = getopt_long(argc, argv, sopt, lopt, &opt_ind)) != -1) {
savevm.c:#define getopt_long_only getopt_long

A few occurence of getopg_long are already in qemu.

Is it a sufficient reason to keep getopt_long ?
If not what would be the alternatives ?

Benoît
> 
> -- 
> Eric Blake   eblake@redhat.com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

Re: [Qemu-devel] [PATCH 3/3] qemu-img: Add json output option to the info command.

[Luiz Capitulino]

On Wed, 15 Aug 2012 16:54:26 +0200
"Benoît Canet" <benoit.canet@gmail.com> wrote:

> 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.

Looks good to me, a few minor comments below.

> 
> example:
> {
>     "snapshots": [
>         {
>             "name": "truc",
>             "date-sec": 1345034924,
>             "date-nsec": 870405000,
>             "vm-clock-sec": 4647590161,
>             "id": "1",
>             "vm-state-size": 80805256
>         },
>         {
>             "name": "onx",
>             "date-sec": 1345034927,
>             "date-nsec": 914633000,
>             "vm-clock-sec": 7065282574,
>             "id": "2",
>             "vm-state-size": 75927370
>         },
>         {
>             "name": "list",
>             "date-sec": 1345034939,
>             "date-nsec": 39119000,
>             "vm-clock-sec": 10108695046,
>             "id": "3",
>             "vm-state-size": 75890567
>         }
>     ],
>     "virtual-size": 1073741824,
>     "filename": "snapshot.img",
>     "cluster-size": 65536,
>     "format": "qcow2",
>     "actual-size": 242614272,
>     "encrypted": false,
>     "backing-filename": "truc5.raw",
>     "dirty-flag": false
> }
> 
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  Makefile   |    3 +-
>  qemu-img.c |  126 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  2 files changed, 118 insertions(+), 11 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index ab82ef3..9ba064b 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -160,7 +160,8 @@ tools-obj-y = $(oslib-obj-y) $(trace-obj-y) qemu-tool.o qemu-timer.o \
>  	iohandler.o cutils.o iov.o async.o
>  tools-obj-$(CONFIG_POSIX) += compatfd.o
>  
> -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-obj-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)
>  
> diff --git a/qemu-img.c b/qemu-img.c
> index 80cfb9b..1a80414 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 DEALINGS 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 <getopt.h>
>  #include <stdio.h>
>  
>  #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"
>             "\n"
>             "Parameters to check subcommand:\n"
>             "  '-r' tries to repair any inconsistencies that are found during the check.\n"
> @@ -1102,10 +1107,66 @@ 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;
> +    SnapshotInfoList *sn_info_list, *cur_item = NULL, *head = NULL;
> +    sn_count = bdrv_snapshot_list(bs, &sn_tab);
> +
> +    for (i = 0; i < sn_count; i++) {
> +        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_sec = sn_tab[i].vm_clock_nsec;
> +
> +        /* XXX: waiting for the qapi to support GSList */
> +        if (!cur_item) {

You can drop head and use imagine_info->snapshots instead.

> +            head = cur_item = sn_info_list;
> +        } else {
> +            cur_item->next = sn_info_list;
> +            cur_item = sn_info_list;
> +        }
> +
> +    }
> +
> +    g_free(sn_tab);
> +    image_info->snapshots = head;
> +}
> +
> +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);
> +    qstring_append_chr(str, '\n');

This call is not needed, just add \n to the printf call below.

> +    printf("%s", 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);
> +
>  static int img_info(int argc, char **argv)
>  {
>      int c;
> -    const char *filename, *fmt;
> +    bool human = true;
> +    const char *filename, *fmt, *machine;
>      BlockDriverState *bs;
>      char size_buf[128], dsize_buf[128];
>      uint64_t total_sectors;
> @@ -1113,10 +1174,20 @@ static int img_info(int argc, char **argv)
>      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 +1199,9 @@ static int img_info(int argc, char **argv)
>          case 'f':
>              fmt = optarg;
>              break;
> +        case 'm':
> +            machine = optarg;
> +            break;
>          }
>      }
>      if (optind >= argc) {
> @@ -1135,6 +1209,11 @@ 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;
> +    }
> +
>      bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_NO_BACKING);
>      if (!bs) {
>          return 1;
> @@ -1148,39 +1227,66 @@ 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;
> +    } else {
> +        image_info->encrypted = false;
>      }
>      if (bdrv_get_info(bs, &bdi) >= 0) {
>          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') {
> +        image_info->backing_filename = g_strdup(backing_filename);
>          bdrv_get_full_backing_filename(bs, backing_filename2,
>                                         sizeof(backing_filename2));
> -        printf("backing file: %s", backing_filename);
> +        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');
>      }
> -    dump_snapshots(bs);
> +
> +    if (human) {
> +        dump_snapshots(bs);
> +    } else {
> +        collect_snapshots(bs, image_info);
> +    }
> +
> +    if (!human) {
> +        dump_json_image_info(image_info);
> +    }

I'd drop that test and move dump_json_image_info() to the else clause
above.

> +
> +    qapi_free_ImageInfo(image_info);
>      bdrv_delete(bs);
>      return 0;
>  }
>  
> +#undef PRINTH
> +
>  #define SNAPSHOT_LIST   1
>  #define SNAPSHOT_CREATE 2
>  #define SNAPSHOT_APPLY  3