Re: [Qemu-devel] [PATCH 07/17] block/qapi: Clean up how we print to monitor or stdout

[Dr. David Alan Gilbert]

* Markus Armbruster (address@hidden) wrote:
> bdrv_snapshot_dump(), bdrv_image_info_specific_dump(),
> bdrv_image_info_dump() and their helpers take an fprintf()-like
> callback and a FILE * to pass to it.
> 
> hmp.c passes monitor_printf() cast to fprintf_function and the current
> monitor cast to FILE *.
> 
> qemu-img.c and qemu-io-cmds.c pass fprintf and stdout.
> 
> The type-punning is technically undefined behaviour, but works in
> practice.  Clean up: drop the callback, and call qemu_printf()
> instead.
> 
> Signed-off-by: Markus Armbruster <address@hidden>

Reviewed-by: Dr. David Alan Gilbert <address@hidden>

> ---
>  block/qapi.c         | 120 ++++++++++++++++++++-----------------------
>  hmp.c                |  12 ++---
>  include/block/qapi.h |  10 ++--
>  qemu-img.c           |   6 +--
>  qemu-io-cmds.c       |   2 +-
>  5 files changed, 67 insertions(+), 83 deletions(-)
> 
> diff --git a/block/qapi.c b/block/qapi.c
> index 21edab34fc..e3e74f898f 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -36,6 +36,7 @@
>  #include "qapi/qmp/qlist.h"
>  #include "qapi/qmp/qnum.h"
>  #include "qapi/qmp/qstring.h"
> +#include "qemu/qemu-print.h"
>  #include "sysemu/block-backend.h"
>  #include "qemu/cutils.h"
>  
> @@ -660,8 +661,7 @@ static char *get_human_readable_size(char *buf, int 
> buf_size, int64_t size)
>      return buf;
>  }
>  
> -void bdrv_snapshot_dump(fprintf_function func_fprintf, void *f,
> -                        QEMUSnapshotInfo *sn)
> +void bdrv_snapshot_dump(QEMUSnapshotInfo *sn)
>  {
>      char buf1[128], date_buf[128], clock_buf[128];
>      struct tm tm;
> @@ -669,9 +669,8 @@ void bdrv_snapshot_dump(fprintf_function func_fprintf, 
> void *f,
>      int64_t secs;
>  
>      if (!sn) {
> -        func_fprintf(f,
> -                     "%-10s%-20s%7s%20s%15s",
> -                     "ID", "TAG", "VM SIZE", "DATE", "VM CLOCK");
> +        qemu_printf("%-10s%-20s%7s%20s%15s",
> +                    "ID", "TAG", "VM SIZE", "DATE", "VM CLOCK");
>      } else {
>          ti = sn->date_sec;
>          localtime_r(&ti, &tm);
> @@ -684,50 +683,46 @@ void bdrv_snapshot_dump(fprintf_function func_fprintf, 
> void *f,
>                   (int)((secs / 60) % 60),
>                   (int)(secs % 60),
>                   (int)((sn->vm_clock_nsec / 1000000) % 1000));
> -        func_fprintf(f,
> -                     "%-10s%-20s%7s%20s%15s",
> -                     sn->id_str, sn->name,
> -                     get_human_readable_size(buf1, sizeof(buf1),
> -                                             sn->vm_state_size),
> -                     date_buf,
> -                     clock_buf);
> +        qemu_printf("%-10s%-20s%7s%20s%15s",
> +                    sn->id_str, sn->name,
> +                    get_human_readable_size(buf1, sizeof(buf1),
> +                                            sn->vm_state_size),
> +                    date_buf,
> +                    clock_buf);
>      }
>  }
>  
> -static void dump_qdict(fprintf_function func_fprintf, void *f, int 
> indentation,
> -                       QDict *dict);
> -static void dump_qlist(fprintf_function func_fprintf, void *f, int 
> indentation,
> -                       QList *list);
> +static void dump_qdict(int indentation, QDict *dict);
> +static void dump_qlist(int indentation, QList *list);
>  
> -static void dump_qobject(fprintf_function func_fprintf, void *f,
> -                         int comp_indent, QObject *obj)
> +static void dump_qobject(int comp_indent, QObject *obj)
>  {
>      switch (qobject_type(obj)) {
>          case QTYPE_QNUM: {
>              QNum *value = qobject_to(QNum, obj);
>              char *tmp = qnum_to_string(value);
> -            func_fprintf(f, "%s", tmp);
> +            qemu_printf("%s", tmp);
>              g_free(tmp);
>              break;
>          }
>          case QTYPE_QSTRING: {
>              QString *value = qobject_to(QString, obj);
> -            func_fprintf(f, "%s", qstring_get_str(value));
> +            qemu_printf("%s", qstring_get_str(value));
>              break;
>          }
>          case QTYPE_QDICT: {
>              QDict *value = qobject_to(QDict, obj);
> -            dump_qdict(func_fprintf, f, comp_indent, value);
> +            dump_qdict(comp_indent, value);
>              break;
>          }
>          case QTYPE_QLIST: {
>              QList *value = qobject_to(QList, obj);
> -            dump_qlist(func_fprintf, f, comp_indent, value);
> +            dump_qlist(comp_indent, value);
>              break;
>          }
>          case QTYPE_QBOOL: {
>              QBool *value = qobject_to(QBool, obj);
> -            func_fprintf(f, "%s", qbool_get_bool(value) ? "true" : "false");
> +            qemu_printf("%s", qbool_get_bool(value) ? "true" : "false");
>              break;
>          }
>          default:
> @@ -735,8 +730,7 @@ static void dump_qobject(fprintf_function func_fprintf, 
> void *f,
>      }
>  }
>  
> -static void dump_qlist(fprintf_function func_fprintf, void *f, int 
> indentation,
> -                       QList *list)
> +static void dump_qlist(int indentation, QList *list)
>  {
>      const QListEntry *entry;
>      int i = 0;
> @@ -744,17 +738,16 @@ static void dump_qlist(fprintf_function func_fprintf, 
> void *f, int indentation,
>      for (entry = qlist_first(list); entry; entry = qlist_next(entry), i++) {
>          QType type = qobject_type(entry->value);
>          bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
> -        func_fprintf(f, "%*s[%i]:%c", indentation * 4, "", i,
> -                     composite ? '\n' : ' ');
> -        dump_qobject(func_fprintf, f, indentation + 1, entry->value);
> +        qemu_printf("%*s[%i]:%c", indentation * 4, "", i,
> +                    composite ? '\n' : ' ');
> +        dump_qobject(indentation + 1, entry->value);
>          if (!composite) {
> -            func_fprintf(f, "\n");
> +            qemu_printf("\n");
>          }
>      }
>  }
>  
> -static void dump_qdict(fprintf_function func_fprintf, void *f, int 
> indentation,
> -                       QDict *dict)
> +static void dump_qdict(int indentation, QDict *dict)
>  {
>      const QDictEntry *entry;
>  
> @@ -769,18 +762,17 @@ static void dump_qdict(fprintf_function func_fprintf, 
> void *f, int indentation,
>              key[i] = entry->key[i] == '-' ? ' ' : entry->key[i];
>          }
>          key[i] = 0;
> -        func_fprintf(f, "%*s%s:%c", indentation * 4, "", key,
> -                     composite ? '\n' : ' ');
> -        dump_qobject(func_fprintf, f, indentation + 1, entry->value);
> +        qemu_printf("%*s%s:%c", indentation * 4, "", key,
> +                    composite ? '\n' : ' ');
> +        dump_qobject(indentation + 1, entry->value);
>          if (!composite) {
> -            func_fprintf(f, "\n");
> +            qemu_printf("\n");
>          }
>          g_free(key);
>      }
>  }
>  
> -void bdrv_image_info_specific_dump(fprintf_function func_fprintf, void *f,
> -                                   ImageInfoSpecific *info_spec)
> +void bdrv_image_info_specific_dump(ImageInfoSpecific *info_spec)
>  {
>      QObject *obj, *data;
>      Visitor *v = qobject_output_visitor_new(&obj);
> @@ -788,13 +780,12 @@ void bdrv_image_info_specific_dump(fprintf_function 
> func_fprintf, void *f,
>      visit_type_ImageInfoSpecific(v, NULL, &info_spec, &error_abort);
>      visit_complete(v, &obj);
>      data = qdict_get(qobject_to(QDict, obj), "data");
> -    dump_qobject(func_fprintf, f, 1, data);
> +    dump_qobject(1, data);
>      qobject_unref(obj);
>      visit_free(v);
>  }
>  
> -void bdrv_image_info_dump(fprintf_function func_fprintf, void *f,
> -                          ImageInfo *info)
> +void bdrv_image_info_dump(ImageInfo *info)
>  {
>      char size_buf[128], dsize_buf[128];
>      if (!info->has_actual_size) {
> @@ -804,49 +795,48 @@ void bdrv_image_info_dump(fprintf_function 
> func_fprintf, void *f,
>                                  info->actual_size);
>      }
>      get_human_readable_size(size_buf, sizeof(size_buf), info->virtual_size);
> -    func_fprintf(f,
> -                 "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);
> +    qemu_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) {
> -        func_fprintf(f, "encrypted: yes\n");
> +        qemu_printf("encrypted: yes\n");
>      }
>  
>      if (info->has_cluster_size) {
> -        func_fprintf(f, "cluster_size: %" PRId64 "\n",
> -                       info->cluster_size);
> +        qemu_printf("cluster_size: %" PRId64 "\n",
> +                    info->cluster_size);
>      }
>  
>      if (info->has_dirty_flag && info->dirty_flag) {
> -        func_fprintf(f, "cleanly shut down: no\n");
> +        qemu_printf("cleanly shut down: no\n");
>      }
>  
>      if (info->has_backing_filename) {
> -        func_fprintf(f, "backing file: %s", info->backing_filename);
> +        qemu_printf("backing file: %s", info->backing_filename);
>          if (!info->has_full_backing_filename) {
> -            func_fprintf(f, " (cannot determine actual path)");
> +            qemu_printf(" (cannot determine actual path)");
>          } else if (strcmp(info->backing_filename,
>                            info->full_backing_filename) != 0) {
> -            func_fprintf(f, " (actual path: %s)", 
> info->full_backing_filename);
> +            qemu_printf(" (actual path: %s)", info->full_backing_filename);
>          }
> -        func_fprintf(f, "\n");
> +        qemu_printf("\n");
>          if (info->has_backing_filename_format) {
> -            func_fprintf(f, "backing file format: %s\n",
> -                         info->backing_filename_format);
> +            qemu_printf("backing file format: %s\n",
> +                        info->backing_filename_format);
>          }
>      }
>  
>      if (info->has_snapshots) {
>          SnapshotInfoList *elem;
>  
> -        func_fprintf(f, "Snapshot list:\n");
> -        bdrv_snapshot_dump(func_fprintf, f, NULL);
> -        func_fprintf(f, "\n");
> +        qemu_printf("Snapshot list:\n");
> +        bdrv_snapshot_dump(NULL);
> +        qemu_printf("\n");
>  
>          /* Ideally bdrv_snapshot_dump() would operate on SnapshotInfoList but
>           * we convert to the block layer's native QEMUSnapshotInfo for now.
> @@ -862,13 +852,13 @@ void bdrv_image_info_dump(fprintf_function 
> func_fprintf, void *f,
>  
>              pstrcpy(sn.id_str, sizeof(sn.id_str), elem->value->id);
>              pstrcpy(sn.name, sizeof(sn.name), elem->value->name);
> -            bdrv_snapshot_dump(func_fprintf, f, &sn);
> -            func_fprintf(f, "\n");
> +            bdrv_snapshot_dump(&sn);
> +            qemu_printf("\n");
>          }
>      }
>  
>      if (info->has_format_specific) {
> -        func_fprintf(f, "Format specific information:\n");
> -        bdrv_image_info_specific_dump(func_fprintf, f, 
> info->format_specific);
> +        qemu_printf("Format specific information:\n");
> +        bdrv_image_info_specific_dump(info->format_specific);
>      }
>  }
> diff --git a/hmp.c b/hmp.c
> index 8eec768088..4bb3af748e 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -580,8 +580,7 @@ static void print_block_info(Monitor *mon, BlockInfo 
> *info,
>          monitor_printf(mon, "\nImages:\n");
>          image_info = inserted->image;
>          while (1) {
> -                bdrv_image_info_dump((fprintf_function)monitor_printf,
> -                                     mon, image_info);
> +                bdrv_image_info_dump(image_info);
>              if (image_info->has_backing_image) {
>                  image_info = image_info->backing_image;
>              } else {
> @@ -1586,7 +1585,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict 
> *qdict)
>      monitor_printf(mon, "List of snapshots present on all disks:\n");
>  
>      if (total > 0) {
> -        bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, NULL);
> +        bdrv_snapshot_dump(NULL);
>          monitor_printf(mon, "\n");
>          for (i = 0; i < total; i++) {
>              sn = &sn_tab[global_snapshots[i]];
> @@ -1594,7 +1593,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict 
> *qdict)
>               * overwrite it.
>               */
>              pstrcpy(sn->id_str, sizeof(sn->id_str), "--");
> -            bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, sn);
> +            bdrv_snapshot_dump(sn);
>              monitor_printf(mon, "\n");
>          }
>      } else {
> @@ -1608,11 +1607,10 @@ void hmp_info_snapshots(Monitor *mon, const QDict 
> *qdict)
>          monitor_printf(mon,
>                         "\nList of partial (non-loadable) snapshots on 
> '%s':\n",
>                         image_entry->imagename);
> -        bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, NULL);
> +        bdrv_snapshot_dump(NULL);
>          monitor_printf(mon, "\n");
>          QTAILQ_FOREACH(snapshot_entry, &image_entry->snapshots, next) {
> -            bdrv_snapshot_dump((fprintf_function)monitor_printf, mon,
> -                               &snapshot_entry->sn);
> +            bdrv_snapshot_dump(&snapshot_entry->sn);
>              monitor_printf(mon, "\n");
>          }
>      }
> diff --git a/include/block/qapi.h b/include/block/qapi.h
> index a891f43b9c..cd9410dee3 100644
> --- a/include/block/qapi.h
> +++ b/include/block/qapi.h
> @@ -27,7 +27,6 @@
>  
>  #include "block/block.h"
>  #include "block/snapshot.h"
> -#include "qemu/fprintf-fn.h"
>  
>  BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
>                                          BlockDriverState *bs, Error **errp);
> @@ -38,10 +37,7 @@ void bdrv_query_image_info(BlockDriverState *bs,
>                             ImageInfo **p_info,
>                             Error **errp);
>  
> -void bdrv_snapshot_dump(fprintf_function func_fprintf, void *f,
> -                        QEMUSnapshotInfo *sn);
> -void bdrv_image_info_specific_dump(fprintf_function func_fprintf, void *f,
> -                                   ImageInfoSpecific *info_spec);
> -void bdrv_image_info_dump(fprintf_function func_fprintf, void *f,
> -                          ImageInfo *info);
> +void bdrv_snapshot_dump(QEMUSnapshotInfo *sn);
> +void bdrv_image_info_specific_dump(ImageInfoSpecific *info_spec);
> +void bdrv_image_info_dump(ImageInfo *info);
>  #endif
> diff --git a/qemu-img.c b/qemu-img.c
> index 55201fb913..ab5e3a2f23 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -2483,11 +2483,11 @@ static void dump_snapshots(BlockDriverState *bs)
>      if (nb_sns <= 0)
>          return;
>      printf("Snapshot list:\n");
> -    bdrv_snapshot_dump(fprintf, stdout, NULL);
> +    bdrv_snapshot_dump(NULL);
>      printf("\n");
>      for(i = 0; i < nb_sns; i++) {
>          sn = &sn_tab[i];
> -        bdrv_snapshot_dump(fprintf, stdout, sn);
> +        bdrv_snapshot_dump(sn);
>          printf("\n");
>      }
>      g_free(sn_tab);
> @@ -2536,7 +2536,7 @@ static void dump_human_image_info_list(ImageInfoList 
> *list)
>          }
>          delim = true;
>  
> -        bdrv_image_info_dump(fprintf, stdout, elem->value);
> +        bdrv_image_info_dump(elem->value);
>      }
>  }
>  
> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> index 09750a23ce..8826bebaf6 100644
> --- a/qemu-io-cmds.c
> +++ b/qemu-io-cmds.c
> @@ -1699,7 +1699,7 @@ static int info_f(BlockBackend *blk, int argc, char 
> **argv)
>      }
>      if (spec_info) {
>          printf("Format specific information:\n");
> -        bdrv_image_info_specific_dump(fprintf, stdout, spec_info);
> +        bdrv_image_info_specific_dump(spec_info);
>          qapi_free_ImageInfoSpecific(spec_info);
>      }
>  
> -- 
> 2.17.2
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK