Re: [Qemu-devel] [PATCH 4/8] monitor: Add basic device state visualization

[Markus Armbruster]

Jan Kiszka <address@hidden> writes:

> This introduces device_show, a monitor command that saves the vmstate of
> a qdev device and visualizes it. QMP is also supported. Buffers are cut
> after 16 byte by default, but the full content can be requested via
> '-f'. To pretty-print sub-arrays, vmstate is extended to store the start
> index name.
>
> Signed-off-by: Jan Kiszka <address@hidden>

Neato!  Comments inline.

> ---
>  hw/hw.h         |    2 +
>  hw/qdev.c       |  287 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/qdev.h       |    2 +
>  qemu-monitor.hx |   16 +++
>  4 files changed, 307 insertions(+), 0 deletions(-)
>
> diff --git a/hw/hw.h b/hw/hw.h
> index 328b704..1ff8e40 100644
> --- a/hw/hw.h
> +++ b/hw/hw.h
> @@ -299,6 +299,7 @@ enum VMStateFlags {
>  
>  typedef struct {
>      const char *name;
> +    const char *start_index;
>      size_t offset;
>      size_t size;
>      size_t start;

Why is start_index a string?

> @@ -414,6 +415,7 @@ extern const VMStateInfo vmstate_info_unused_buffer;
>      .size       = sizeof(_type),                                     \
>      .flags      = VMS_ARRAY,                                         \
>      .offset     = vmstate_offset_sub_array(_state, _field, _type, _start), \
> +    .start_index = (stringify(_start)),                              \
>  }
>  
>  #define VMSTATE_VARRAY_INT32(_field, _state, _field_num, _version, _info, 
> _type) {\
> diff --git a/hw/qdev.c b/hw/qdev.c
> index fe49e71..c989010 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -29,6 +29,9 @@
>  #include "qdev.h"
>  #include "sysemu.h"
>  #include "monitor.h"
> +#include "qjson.h"
> +#include "qint.h"
> +#include "qbuffer.h"
>  
>  static int qdev_hotplug = 0;
>  
> @@ -824,3 +827,287 @@ int do_device_del(Monitor *mon, const QDict *qdict, 
> QObject **ret_data)
>      }
>      return qdev_unplug(dev);
>  }
> +
> +static int print_array_index(Monitor *mon, const char *start, int index)
> +{
> +    char index_str[32];
> +    int len;
> +
> +    if (start) {
> +        len = snprintf(index_str, sizeof(index_str), "[%s+%02x]", start,
> +                       index);
> +    } else {
> +        len = snprintf(index_str, sizeof(index_str), "[%02x]", index);
> +    }
> +    monitor_printf(mon, index_str);
> +    return len;

You go via the snprintf() detour just because monitor_printf() doesn't
return the number of characters printed.  Wouldn't it be better to fix
that?

> +}
> +
> +#define NAME_COLUMN_WIDTH 23
> +
> +static void print_field(Monitor *mon, const QDict *qfield, int indent);
> +
> +static void print_elem(Monitor *mon, const QObject *qelem, size_t size,
> +                       int column_pos, int indent)
> +{
> +    int64_t data_size;
> +    const void *data;
> +    int n;
> +
> +    if (qobject_type(qelem) == QTYPE_QDICT) {
> +        if (column_pos >= 0) {
> +            monitor_printf(mon, ".\n");
> +        }
> +    } else {
> +        monitor_printf(mon, ":");
> +        column_pos++;
> +        if (column_pos < NAME_COLUMN_WIDTH) {
> +            monitor_printf(mon, "%*c", NAME_COLUMN_WIDTH - column_pos, ' ');
> +        }
> +    }
> +
> +    switch (qobject_type(qelem)) {
> +    case QTYPE_QDICT:
> +        print_field(mon, qobject_to_qdict(qelem), indent + 2);
> +        break;
> +    case QTYPE_QBUFFER:
> +        data = qbuffer_get_data(qobject_to_qbuffer(qelem));
> +        data_size = qbuffer_get_size(qobject_to_qbuffer(qelem));
> +        for (n = 0; n < data_size; ) {
> +            monitor_printf(mon, " %02x", *((uint8_t *)data+n));
> +            if (++n < size) {
> +                if (n % 16 == 0) {
> +                    monitor_printf(mon, "\n%*c", NAME_COLUMN_WIDTH, ' ');
> +                } else if (n % 8 == 0) {
> +                    monitor_printf(mon, " -");
> +                }
> +            }
> +        }
> +        if (data_size < size) {
> +            monitor_printf(mon, " ...");
> +        }
> +        monitor_printf(mon, "\n");
> +        break;
> +    case QTYPE_QINT:
> +        monitor_printf(mon, " %0*x\n", (int)size * 2,
> +                       (int)qint_get_int(qobject_to_qint(qelem)));

I'm confused.  Doesn't casting qint_get_int() to int lose bits?

> +        break;
> +    default:
> +        assert(0);
> +    }
> +}
> +
> +static void print_field(Monitor *mon, const QDict *qfield, int indent)
> +{
> +    const char *name = qdict_get_str(qfield, "name");
> +    const char *start = qdict_get_try_str(qfield, "start");
> +    int64_t size = qdict_get_int(qfield, "size");
> +    QList *qlist = qdict_get_qlist(qfield, "elems");
> +    QListEntry *entry, *sub_entry;
> +    QList *sub_list;
> +    int elem_no = 0;
> +
> +    QLIST_FOREACH_ENTRY(qlist, entry) {
> +        QObject *qelem = qlist_entry_obj(entry);
> +        int pos = indent + strlen(name);
> +
> +        if (qobject_type(qelem) == QTYPE_QLIST) {
> +            monitor_printf(mon, "%*c%s", indent, ' ', name);
> +            pos += print_array_index(mon, start, elem_no);
> +            sub_list = qobject_to_qlist(qelem);
> +            QLIST_FOREACH_ENTRY(sub_list, sub_entry) {
> +                print_elem(mon, qlist_entry_obj(sub_entry), size, pos,
> +                           indent + 2);
> +                pos = -1;
> +            }
> +        } else {
> +            if (elem_no == 0) {
> +                monitor_printf(mon, "%*c%s", indent, ' ', name);
> +            } else {
> +                pos = -1;
> +            }
> +            print_elem(mon, qelem, size, pos, indent);
> +        }
> +        elem_no++;
> +    }
> +}
> +
> +void device_user_print(Monitor *mon, const QObject *data)
> +{
> +    QDict *qdict = qobject_to_qdict(data);
> +    QList *qlist = qdict_get_qlist(qdict, "fields");
> +    QListEntry *entry;
> +
> +    monitor_printf(mon, "dev: %s, id \"%s\"\n",
> +                   qdict_get_str(qdict, "device"),
> +                   qdict_get_str(qdict, "id"));
> +
> +    QLIST_FOREACH_ENTRY(qlist, entry) {
> +        print_field(mon, qobject_to_qdict(qlist_entry_obj(entry)), 2);
> +    }
> +}
> +
> +static void parse_vmstate(const VMStateDescription *vmsd, void *opaque,
> +                          QList *qlist, int full_buffers)
> +{
> +    VMStateField *field = vmsd->fields;
> +
> +    if (vmsd->pre_save) {
> +        vmsd->pre_save(opaque);
> +    }
> +    while(field->name) {
> +        if (!field->field_exists ||
> +            field->field_exists(opaque, vmsd->version_id)) {
> +            void *base_addr = opaque + field->offset;
> +            int i, n_elems = 1;
> +            int is_array = 1;
> +            size_t size = field->size;
> +            QDict *qfield = qdict_new();
> +            QList *qelems = qlist_new();
> +
> +            qlist_append_obj(qlist, QOBJECT(qfield));
> +
> +            qdict_put_obj(qfield, "name",
> +                          QOBJECT(qstring_from_str(field->name)));
> +            qdict_put_obj(qfield, "elems", QOBJECT(qelems));
> +
> +            if (field->flags & VMS_VBUFFER) {
> +                size = *(int32_t *)(opaque + field->size_offset);
> +                if (field->flags & VMS_MULTIPLY) {
> +                    size *= field->size;
> +                }
> +            }
> +            qdict_put_obj(qfield, "size", QOBJECT(qint_from_int(size)));
> +            if (field->start_index) {
> +                qdict_put_obj(qfield, "start",
> +                              QOBJECT(qstring_from_str(field->start_index)));
> +            }
> +
> +            if (field->flags & VMS_ARRAY) {
> +                n_elems = field->num;
> +            } else if (field->flags & VMS_VARRAY_INT32) {
> +                n_elems = *(int32_t *)(opaque + field->num_offset);
> +            } else if (field->flags & VMS_VARRAY_UINT16) {
> +                n_elems = *(uint16_t *)(opaque + field->num_offset);
> +            } else {
> +                is_array = 0;
> +            }
> +            if (field->flags & VMS_POINTER) {
> +                base_addr = *(void **)base_addr + field->start;
> +            }
> +            for (i = 0; i < n_elems; i++) {
> +                void *addr = base_addr + size * i;
> +                QList *sub_elems = qelems;
> +                int val;
> +
> +                if (is_array) {
> +                    sub_elems = qlist_new();
> +                    qlist_append_obj(qelems, QOBJECT(sub_elems));
> +                }
> +                if (field->flags & VMS_ARRAY_OF_POINTER) {
> +                    addr = *(void **)addr;
> +                }
> +                if (field->flags & VMS_STRUCT) {
> +                    parse_vmstate(field->vmsd, addr, sub_elems, 
> full_buffers);
> +                } else {
> +                    if (field->flags & (VMS_BUFFER | VMS_VBUFFER)) {
> +                        if (!full_buffers && size > 16) {
> +                            size = 16;
> +                        }
> +                        qlist_append_obj(sub_elems,
> +                                         QOBJECT(qbuffer_from_data(addr,
> +                                                                   size)));
> +                    } else {
> +                        switch (size) {
> +                        case 1:
> +                            val = *(uint8_t *)addr;
> +                            break;
> +                        case 2:
> +                            val = *(uint16_t *)addr;
> +                            break;
> +                        case 4:
> +                            val = *(uint32_t *)addr;
> +                            break;
> +                        case 8:
> +                            val = *(uint64_t *)addr;
> +                            break;
> +                        default:
> +                            assert(0);
> +                        }
> +                        qlist_append_obj(sub_elems,
> +                                         QOBJECT(qint_from_int(val)));
> +                    }
> +                }
> +            }
> +        }
> +        field++;
> +    }
> +}
> +
> +static DeviceState *qdev_find(const char *path)
> +{
> +    const char *dev_name;
> +    DeviceState *dev;
> +    char *bus_path;
> +    BusState *bus;
> +
> +    dev_name = strrchr(path, '/');
> +    if (!dev_name) {
> +        bus = main_system_bus;
> +        dev_name = path;
> +    } else {
> +        dev_name++;
> +        bus_path = qemu_strdup(path);
> +        bus_path[dev_name - path] = 0;
> +
> +        bus = qbus_find(bus_path);
> +        qemu_free(bus_path);
> +
> +        if (!bus) {
> +            /* qbus_find already reported the error */
> +            return NULL;
> +        }
> +    }
> +    dev = qbus_find_dev(bus, dev_name);
> +    if (!dev) {
> +        qerror_report(QERR_DEVICE_NOT_FOUND, dev_name);
> +        if (!monitor_cur_is_qmp()) {
> +            qbus_list_dev(bus);
> +        }
> +    }
> +    return dev;
> +}
> +
> +int do_device_show(Monitor *mon, const QDict *qdict, QObject **ret_data)

Command documentation missing.

> +{
> +    const char *path = qdict_get_str(qdict, "path");
> +    const VMStateDescription *vmsd;
> +    DeviceState *dev;
> +    QList *qlist;
> +
> +    dev = qdev_find_recursive(main_system_bus, path);
> +    if (!dev) {
> +        dev = qdev_find(path);
> +        if (!dev) {
> +            return -1;
> +        }
> +    }

This is inconsistent with how do_device_del() looks up devices by ID.
Let's agree on one lookup, put it into a function, and use it
everywhere.

> +
> +    vmsd = dev->info->vmsd;
> +    if (!vmsd) {
> +        qerror_report(QERR_UNDEFINED_ERROR);
> +        if (!monitor_cur_is_qmp()) {
> +            error_printf_unless_qmp("Device '%s' does not support state "
> +                                    "dumping\n", path);
> +        }

Please use a suitable QERR_.  Define one if necessary.

> +        return -1;
> +    }
> +
> +    *ret_data = qobject_from_jsonf("{ 'device': %s, 'id': %s }",
> +                                   dev->info->name, dev->id ? : "");
> +    qlist = qlist_new();
> +    parse_vmstate(vmsd, dev, qlist, qdict_get_int(qdict, "full"));
> +    qdict_put_obj(qobject_to_qdict(*ret_data), "fields", QOBJECT(qlist));
> +
> +    return 0;
> +}
> diff --git a/hw/qdev.h b/hw/qdev.h
> index d8fbc73..f8436ec 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -177,6 +177,8 @@ void do_info_qtree(Monitor *mon);
>  void do_info_qdm(Monitor *mon);
>  int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data);
>  int do_device_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
> +void device_user_print(Monitor *mon, const QObject *data);
> +int do_device_show(Monitor *mon, const QDict *qdict, QObject **ret_data);
>  
>  /*** qdev-properties.c ***/
>  
> diff --git a/qemu-monitor.hx b/qemu-monitor.hx
> index a8f194c..449f012 100644
> --- a/qemu-monitor.hx
> +++ b/qemu-monitor.hx
> @@ -599,6 +599,22 @@ Remove device @var{id}.
>  ETEXI
>  
>      {
> +        .name       = "device_show",
> +        .args_type  = "path:s,full:-f",
> +        .params     = "device [-f]",
> +        .help       = "show device state (specify -f for full buffer 
> dumping)",
> +        .user_print = device_user_print,
> +        .mhandler.cmd_new = do_device_show,
> +    },
> +
> +STEXI
> address@hidden device_show @var{id} address@hidden
> +
> +Show state of device @var{id} in a human-readable form. Buffers are cut after
> +16 bytes unless a full dump is requested via @code{-f} 
> +ETEXI
> +
> +    {
>          .name       = "cpu",
>          .args_type  = "index:i",
>          .params     = "index",