Thanks for reviewing.
I will correct the series tomorow.
Benoît
Le Tuesday 04 Sep 2012 à 14:54:18 (+0200), Kevin Wolf a écrit :
> Am 27.08.2012 09:15, schrieb Benoît Canet:
> > This option --output=[human|json] make qemu-img info output on
> > human or JSON representation at the choice of the user.
> >
> > 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
> > }
> >
> > Signed-off-by: Benoit Canet <ben...@irqsave.net>
> > ---
> > Makefile | 3 +-
> > qemu-img.c | 259
> > ++++++++++++++++++++++++++++++++++++++++++++++++++----------
> > 2 files changed, 218 insertions(+), 44 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..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 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"
> > + " '--output' takes the format in which the output must be done
> > (human or json)\n"
> > "\n"
> > "Parameters to check subcommand:\n"
> > " '-r' tries to repair any inconsistencies that are found
> > during the check.\n"
>
> qemu-img-cmds.hx and qemu-img.texi should be updated with documentation
> of the new option as well.
>
> > @@ -1102,21 +1107,196 @@ static void dump_snapshots(BlockDriverState *bs)
> > g_free(sn_tab);
> > }
> >
> > -static int img_info(int argc, char **argv)
> > +static void collect_snapshots(BlockDriverState *bs , ImageInfo *info)
> > +{
> > + int i, sn_count;
> > + QEMUSnapshotInfo *sn_tab = NULL;
> > + SnapshotInfoList *info_list, *cur_item = NULL;
> > + sn_count = bdrv_snapshot_list(bs, &sn_tab);
> > +
> > + for (i = 0; i < sn_count; i++) {
> > + info->has_snapshots = true;
> > + info_list = g_new0(SnapshotInfoList, 1);
> > +
> > + info_list->value = g_new0(SnapshotInfo, 1);
> > + info_list->value->id = g_strdup(sn_tab[i].id_str);
> > + info_list->value->name = g_strdup(sn_tab[i].name);
> > + info_list->value->vm_state_size = sn_tab[i].vm_state_size;
> > + info_list->value->date_sec = sn_tab[i].date_sec;
> > + info_list->value->date_nsec = sn_tab[i].date_nsec;
> > + info_list->value->vm_clock_sec = sn_tab[i].vm_clock_nsec /
> > 1000000000;
> > + info_list->value->vm_clock_nsec = sn_tab[i].vm_clock_nsec %
> > 1000000000;
> > +
> > + /* XXX: waiting for the qapi to support GSList */
>
> No, GSList isn't typesafe and therefore to be avoided. If anything, the
> support we want is for qemu-queue.h types.
>
> > + if (!cur_item) {
> > + info->snapshots = cur_item = info_list;
> > + } else {
> > + cur_item->next = info_list;
> > + cur_item = info_list;
> > + }
> > +
> > + }
> > +
> > + g_free(sn_tab);
> > +}
> > +
> > +static void dump_json_image_info(ImageInfo *info)
> > +{
> > + Error *errp = NULL;
> > + QString *str;
> > + QmpOutputVisitor *ov = qmp_output_visitor_new();
> > + QObject *obj;
> > + visit_type_ImageInfo(qmp_output_get_visitor(ov),
> > + &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);
> > +}
> > +
> > +static void collect_backing_file_format(ImageInfo *info, char *filename)
> > +{
> > + BlockDriverState *bs = NULL;
> > + bs = bdrv_new_open(filename, NULL,
> > + BDRV_O_FLAGS | BDRV_O_NO_BACKING);
> > + if (!bs) {
> > + return;
> > + }
> > + info->backing_filename_format =
> > + g_strdup(bdrv_get_format_name(bs));
> > + bdrv_delete(bs);
> > + info->has_backing_filename_format = true;
> > +}
>
> This is new functionality and should be added in a separate patch.
>
> It is also wrong because it uses NULL instead of the explicitly
> specified format to open the image.
>
> > +
> > +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 = g_strdup(filename);
> > + info->format = g_strdup(bdrv_get_format_name(bs));
> > + info->virtual_size = total_sectors * 512;
> > + info->actual_size = bdrv_get_allocated_file_size(bs);
> > + info->has_actual_size = info->actual_size >= 0;
> > + if (bdrv_is_encrypted(bs)) {
> > + info->encrypted = true;
> > + info->has_encrypted = true;
> > + }
> > + if (bdrv_get_info(bs, &bdi) >= 0) {
> > + if (bdi.cluster_size != 0) {
> > + info->cluster_size = bdi.cluster_size;
> > + info->has_cluster_size = true;
> > + }
> > + if (bdi.is_dirty) {
> > + info->dirty_flag = true;
> > + } else {
> > + info->dirty_flag = false;
> > + }
>
> Shorter written as:
>
> info->dirty_flag = bdi.is_dirty;
>
> > + info->has_dirty_flag = true;
> > + }
> > + bdrv_get_backing_filename(bs, backing_filename,
> > sizeof(backing_filename));
> > + if (backing_filename[0] != '\0') {
> > + info->backing_filename = g_strdup(backing_filename);
> > + info->has_backing_filename = true;
> > + bdrv_get_full_backing_filename(bs, backing_filename2,
> > + sizeof(backing_filename2));
> > +
> > + if (strcmp(backing_filename, backing_filename2) != 0) {
> > + info->full_backing_filename =
> > + g_strdup(backing_filename2);
> > + info->has_full_backing_filename = true;
> > + }
> > +
> > + if (access(backing_filename2, R_OK) == 0 &&
> > + !stat(backing_filename2, &st) &&
> > + S_ISREG(st.st_mode)) {
>
> What are you doing here? This is most certainly wrong because it doesn't
> consider protocols supported by qemu.
>
> 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.
>
> > + 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->virtual_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");
> > + }
> > +
>
> > + 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');
> > + }
>
> The full filename is only present if the "normal" name is present
> (otherwise the output would come out wrong anyway). So:
>
> 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');
> }
>
> > +}
> > +
> > +enum {OPTION_OUTPUT = 256};
> > +
> > +typedef enum Format {
> > + FORMAT_JSON,
> > + FORMAT_HUMAN,
> > +} Format;
>
> Hm, calling this Format is asking for naming conflicts. Maybe OutputFormat?
>
> > +
> > +static int img_info(int argc, char **argv)
> > +{
> > + int c;
> > + Format output_format;
>
> Format output_format = FORMAT_HUMAN;
>
> > + const char *filename, *fmt, *output;
> > + BlockDriverState *bs;
> > + ImageInfo *info;
> >
> > fmt = NULL;
> > + output = NULL;
> > for(;;) {
> > - c = getopt(argc, argv, "f:h");
> > + int option_index = 0;
> > + static const struct option long_options[] = {
> > + {"help", no_argument, 0, 'h'},
> > + {"format", required_argument, 0, 'f'},
> > + {"output", required_argument, 0, OPTION_OUTPUT},
> > + {0, 0, 0, 0}
> > + };
> > + c = getopt_long(argc, argv, "f:h",
> > + long_options, &option_index);
> > if (c == -1) {
> > break;
> > }
> > @@ -1128,6 +1308,9 @@ static int img_info(int argc, char **argv)
> > case 'f':
> > fmt = optarg;
> > break;
> > + case OPTION_OUTPUT:
> > + output = optarg;
> > + break;
> > }
> > }
> > if (optind >= argc) {
> > @@ -1135,48 +1318,38 @@ static int img_info(int argc, char **argv)
> > }
> > filename = argv[optind++];
> >
> > - bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_NO_BACKING);
> > - if (!bs) {
> > + if (output && !strcmp(output, "json")) {
> > + output_format = FORMAT_JSON;
> > + } else if (output && !strcmp(output, "human")) {
> > + output_format = FORMAT_HUMAN;
> > + } else if (output) {
> > + fprintf(stderr,
> > + "Error: --output must be used with human or json as
> > argument.\n");
>
> error_report()?
>
> > return 1;
> > - }
> > - bdrv_get_geometry(bs, &total_sectors);
> > - get_human_readable_size(size_buf, sizeof(size_buf), total_sectors *
> > 512);
> > - allocated_size = 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 = FORMAT_HUMAN;
> > }
>
> When you add the default above, you don't need the else branch here.
>
> > - 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 = g_new0(ImageInfo, 1);
> > + bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_NO_BACKING);
> > + if (!bs) {
> > + g_free(info);
> > + return 1;
> > }
> > - if (bdrv_get_info(bs, &bdi) >= 0) {
> > - if (bdi.cluster_size != 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 == FORMAT_HUMAN) {
> > + dump_human_image_info(info);
> > + dump_snapshots(bs);
> > }
> > - 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);
> > - if (strcmp(backing_filename, backing_filename2) != 0) {
> > - printf(" (actual path: %s)", backing_filename2);
> > - }
> > - putchar('\n');
> > +
> > + if (output_format == FORMAT_JSON) {
> > + collect_snapshots(bs, info);
> > + dump_json_image_info(info);
> > }
>
> Use a switch instead of two independent ifs?
>
> > - dump_snapshots(bs);
> > +
> > + qapi_free_ImageInfo(info);
> > bdrv_delete(bs);
> > return 0;
> > }
> >
>
> Kevin
