[Qemu-devel] Re: [PATCH v3 11/12] monitor/net: Convert do_info_network() to QObject/QMP

[Luiz Capitulino <lcapitulino@redhat.com>]

On Thu, 15 Apr 2010 11:07:06 -0300
Miguel Di Ciurcio Filho <miguel.filho@gmail.com> wrote:

> Each device is represented by a QDict. The returned QObject is a QList
> of all devices.
> 
> This commit slightly changes the monitor output when 'info network' is used.

 Actually, it's much more than that.

 For example, the socket (connection type) was:

(qemu) info network
VLAN 0 devices:
  foobar.1: socket: connection from 127.0.0.1:41383
Devices not on any VLAN:
(qemu)

 Now it became:

(qemu) info network
Devices on VLANs:
  foobar.1: vlan=0 family=ipv4 service=41390 host=127.0.0.1  
Devices not on any VLAN:
(qemu)

 So, we're degrading the user Monitor here.

 My suggestion is:

 1. If a device already print "low-level" info (eg. slirp), then it's
    ok to just dump the dict. Actually, this is going to be the default
    behavior for new devices (when we get them)

 2. If a device prints a user-friendly formatted string, then we'll have to
    maintain it (eg. socket). You can do that by checking the device
    model in the iterator and formatting the user-friendly string
    accordingly.

    Another possibility is to have a 'user_str' in VLANClientState, which
    can be formatted at 'info_dict' creation time.

 Also, this change:

> Old output:
>   (qemu) info network
>   VLAN 1 devices:
>     e1000.0: model=e1000,macaddr=52:54:00:12:34:56
>   VLAN 0 devices:
>     tap.0: ifname=tap0,script=/etc/kvm/kvm-ifup,downscript=/etc/qemu-ifdown
>   Devices not on any VLAN:
> 
> New output:
>   (qemu) info network
>   Devices on VLANs:
>     e1000.0: vlan=1 model=e1000 macaddr=52:54:00:12:34:56
>     tap.0: vlan=0 script=/etc/kvm/kvm-ifup downscript=/etc/qemu-ifdown ifname=tap0
>   Devices not on any VLAN:

 doesn't look so good either, as 'info network' won't be that useful if we
get several devices and several vlans. Not sure whether this is a real scenario,
but at least the current code supports it.

 An (inefficient) way to maintain the old format is:

1. Add a key with the number of vlans to the dict (say 'vlans_total')

2. In do_info_network_print() loop using vlans_total in the conditional,
   like this:

   for (i = 0; i < vlans_total; i++) {
        monitor_printf(mon, "VLAN %d devices\n", i);
        qlist_iter(...)
   }

   Also put 'i' either into a global variable or allocate a struct to pass
   it and 'mon' down

3. In vlan_devices_iter() only print devices that correspond to vlan 'i'

> 
> Signed-off-by: Miguel Di Ciurcio Filho <miguel.filho@gmail.com>
> ---
>  monitor.c |    3 +-
>  net.c     |  127 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
>  net.h     |    3 +-
>  3 files changed, 122 insertions(+), 11 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 4c6275e..34781ba 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2610,7 +2610,8 @@ static const mon_cmd_t info_cmds[] = {
>          .args_type  = "",
>          .params     = "",
>          .help       = "show the network state",
> -        .mhandler.info = do_info_network,
> +        .user_print = do_info_network_print,
> +        .mhandler.info_new = do_info_network,
>      },
>      {
>          .name       = "chardev",
> diff --git a/net.c b/net.c
> index 8c02f28..46e8873 100644
> --- a/net.c
> +++ b/net.c
> @@ -35,6 +35,8 @@
>  #include "sysemu.h"
>  #include "qemu-common.h"
>  #include "qemu_socket.h"
> +#include "qemu-objects.h"
> +#include "qobject.h"
>  #include "qdict.h"
>  #include "qstring.h"
>  #include "hw/qdev.h"
> @@ -1285,28 +1287,135 @@ void net_set_boot_mask(int net_boot_mask)
>      }
>  }
>  
> -void do_info_network(Monitor *mon)
> +static void vlan_devices_iter(QObject *obj, void *opaque)
> +{
> +
> +    Monitor *mon = opaque;
> +    QDict *net_device = qobject_to_qdict(obj);
> +
> +    if (!qdict_haskey(net_device, "vlan"))
> +        return;
> +
> +    monitor_printf(mon, "  %s: vlan=%d ", qdict_get_str(net_device, "name"),
> +        (int)qdict_get_int(net_device, "vlan"));
> +    monitor_printf(mon,
> +        qstring_get_str(qdict_to_qstring(qdict_get_qdict(net_device, "info"))));
> +
> +    monitor_printf(mon, " \n");
> +}
> +
> +static void non_vlan_devices_iter(QObject *obj, void *opaque)
> +{
> +
> +    Monitor *mon = opaque;
> +    QDict *net_device = qobject_to_qdict(obj);
> +
> +    if (qdict_haskey(net_device, "vlan"))
> +        return;
> +
> +    monitor_printf(mon, "  %s: ", qdict_get_str(net_device, "name"));
> +
> +    if (qdict_haskey(net_device, "peer"))
> +        monitor_printf(mon, "peer=%s ", qdict_get_str(net_device, "peer"));
> +
> +    monitor_printf(mon,
> +        qstring_get_str(qdict_to_qstring(qdict_get_qdict(net_device, "info"))));
> +
> +    monitor_printf(mon, "\n");
> +
> +}
> +
> +void do_info_network_print(Monitor *mon, const QObject *ret_data)
> +{
> +    QList *qlist;
> +
> +    qlist = qobject_to_qlist(ret_data);
> +
> +    monitor_printf(mon, "Devices on VLANs:\n");
> +
> +    qlist_iter(qlist, vlan_devices_iter, mon);
> +
> +    monitor_printf(mon, "Devices not on any VLAN:\n");
> +
> +    qlist_iter(qlist, non_vlan_devices_iter, mon);
> +
> +}
> +
> +/**
> + * do_network_info(): Network information

 It's do_info_network()

> + *
> + * Each network device information is stored in a QDict and the
> + * returned QObject is a QList of all devices.
> + *
> + * The QDict contains the following:
> + *
> + * - "name": device name

 Isn't 'name' the device id?

> + * - "vlan": only present if the device is attached to a VLAN, it is the id
> + * of the VLAN

 Please, fix the indentation, like:

* - "vlan": only present if the device is attached to a VLAN, it is the id
            of the VLAN

> + * - "info": it is a QDict that may contain any of the following, depending on
> + * the type of the device:
> + *          - "model": type of the device
> + *          - "macaddr": MAC address
> + *          - "script": path to script used to configure the device
> + *          - "downscript": path to script used to deconfigure the device
> + *          - "fd": handle to the device
> + *          - "ifname": name of the host device connected to the guest device
> + *          - "host": IP address of a socket
> + *          - "service": port of a socket
> + *          - "family": Internet protocol family (IPv4 or IPv6)
> + *
> + * Example:
> + *
> + * [ { "name": "tap.0", "vlan": 0, 
> +       "info": { "script": "/etc/kvm/kvm-ifup", "downscript":
> +"/etc/qemu-ifdown", 
> +       "ifname": "tap0" } }, 
> +     { "name": "e1000.0", "vlan": 1,
> +      "info": { "model": "e1000", "macaddr": "52:54:00:12:34:56" } } ]
> + */

 Please, indent this, like bdrv_info().

> +void do_info_network(Monitor *mon, QObject **ret_data)
>  {
>      VLANState *vlan;
>      VLANClientState *vc;
> +    QDict *net_device;
> +    QList *device_list;
> +    device_list = qlist_new();
>  
>      QTAILQ_FOREACH(vlan, &vlans, next) {
> -        monitor_printf(mon, "VLAN %d devices:\n", vlan->id);
> +        QObject *obj;

 This is also used by the other loop, let's declare it as a function
variable to avoid duplication.

>  
>          QTAILQ_FOREACH(vc, &vlan->clients, next) {
> -            monitor_printf(mon, "  %s: %s\n", vc->name, vc->info_str);
> +
> +            obj = qobject_from_jsonf("{ 'vlan': %d, 'name': %s }", vlan->id,
> +vc->name);
> +            net_device = qobject_to_qdict(obj);
> +
> +            QINCREF(vc->info_dict);
> +            qdict_put(net_device, "info", vc->info_dict);
> +
> +            qlist_append(device_list, qobject_to_qdict(obj));
> +
>          }
>      }
> -    monitor_printf(mon, "Devices not on any VLAN:\n");
> +
>      QTAILQ_FOREACH(vc, &non_vlan_clients, next) {
> -        monitor_printf(mon, "  %s: %s", vc->name, vc->info_str);
> -        if (vc->peer) {
> -            monitor_printf(mon, " peer=%s", vc->peer->name);
> -        }
> -        monitor_printf(mon, "\n");
> +        QObject *obj;
> +        obj = qobject_from_jsonf("{ 'name': %s }", vc->name);
> +        net_device = qobject_to_qdict(obj);
> +
> +        QINCREF(vc->info_dict);
> +        qdict_put(net_device, "info", vc->info_dict);
> +
> +        if (vc->peer)
> +            qdict_put(net_device, "peer", qstring_from_str(vc->peer->name));

 Did you check if 'peer' is QMPable?

> +
> +        qlist_append(device_list, net_device);
>      }
> +
> +    *ret_data = QOBJECT(device_list);
>  }
>  
> +
>  int do_set_link(Monitor *mon, const QDict *qdict, QObject **ret_data)
>  {
>      VLANState *vlan;
> diff --git a/net.h b/net.h
> index d12276a..058420e 100644
> --- a/net.h
> +++ b/net.h
> @@ -119,7 +119,8 @@ void qemu_check_nic_model(NICInfo *nd, const char *model);
>  int qemu_find_nic_model(NICInfo *nd, const char * const *models,
>                          const char *default_model);
>  
> -void do_info_network(Monitor *mon);
> +void do_info_network_print(Monitor *mon, const QObject *ret_data);
> +void do_info_network(Monitor *mon, QObject **ret_data);
>  int do_set_link(Monitor *mon, const QDict *qdict, QObject **ret_data);
>  
>  /* NIC info */