[Qemu-devel] Re: [PATCH] net: Convert do_info_network() to QObject

[Luiz Capitulino <lcapitulino@redhat.com>]

On Sat,  3 Apr 2010 18:32:01 -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 should not change user output.
> 
> Signed-off-by: Miguel Di Ciurcio Filho <miguel.filho@gmail.com>
> ---
> 
> This is my initial contribution, aiming at participating in Summer of Code.
> 
>  monitor.c |    3 +-
>  net.c     |   75 ++++++++++++++++++++++++++++++++++++++++++++++++++++---------
>  net.h     |    4 ++-
>  3 files changed, 69 insertions(+), 13 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 822dc27..0b3fdfc 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2604,7 +2604,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 3ede738..7698b0c 100644
> --- a/net.c
> +++ b/net.c
> @@ -35,6 +35,7 @@
>  #include "sysemu.h"
>  #include "qemu-common.h"
>  #include "qemu_socket.h"
> +#include "qemu-objects.h"
>  #include "hw/qdev.h"
>  
>  static QTAILQ_HEAD(, VLANState) vlans;
> @@ -1218,26 +1219,78 @@ void net_set_boot_mask(int net_boot_mask)
>      }
>  }
>  
> -void do_info_network(Monitor *mon)
> +void do_info_network_print(Monitor *mon, const QObject *ret_data)
> +{
> +    QList *qlist;
> +    QListEntry *entry;
> +    QDict *net_device;
> +    
> +    qlist = qobject_to_qlist(ret_data);
> +    net_device = qdict_new();

 Why allocating a new qdict here?

> +
> +    QTAILQ_FOREACH(entry, &qlist->head, next) {

 While I understand that this is convenient, it assumes (and exposes) that
qlist uses QTAILQ internally, this code will break if we change to
something else.

 Better to use qlist's own transverse functions (please, check qlist.h).

 (to be honest, it's arguable if we should allow that, but let's make
  this commit do what the current code does)

> +        net_device = qobject_to_qdict(entry->value);
> +        
> +        if (!qdict_haskey(net_device, "vlan"))
> +            continue;
> +
> +        net_device = qobject_to_qdict(entry->value);
> +        monitor_printf(mon, "VLAN %d devices:\n", (int)qdict_get_int(net_device, "vlan"));
> +        monitor_printf(mon, "  %s: %s\n", qdict_get_str(net_device, "name"), qdict_get_str(net_device, "info"));
> +    }

 You're printing the header "VLAN %d devices:" for each device, please
move it outside the loop.

 Also, as we have talked in private, 'info_str' should be a qdict (and
not a string to be parsed).

> +
> +    monitor_printf(mon, "Devices not on any VLAN:\n");
> +    
> +    QTAILQ_FOREACH(entry, &qlist->head, next) {
> +        net_device = qobject_to_qdict(entry->value);
> +        
> +        if (qdict_haskey(net_device, "vlan"))
> +            continue;
> +        
> +        net_device = qobject_to_qdict(entry->value);
> +        monitor_printf(mon, "  %s: %s", qdict_get_str(net_device, "name"), qdict_get_str(net_device, "info"));
> +    
> +        if (qdict_haskey(net_device, "peer"))
> +            monitor_printf(mon, " peer=%s", qdict_get_str(net_device, "peer"));
> +        
> +        monitor_printf(mon, "\n");
> +    }
> +
> +}
> +
> +void do_info_network(Monitor *mon, QObject **ret_data)
>  {

 Please, add documentation as in bdrv_info() and others converted functions.

>      VLANState *vlan;
>      VLANClientState *vc;
> -
> +    QList *device_list;
> +    device_list = qlist_new();
> +        
>      QTAILQ_FOREACH(vlan, &vlans, next) {
> -        monitor_printf(mon, "VLAN %d devices:\n", vlan->id);
> -
> +        QObject *obj;
> +        
>          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, 'info': %s}", vlan->id, vc->name, vc->info_str);
> +
> +            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;
> +        QDict *net_device;
> +        
> +        obj = qobject_from_jsonf("{ 'name': %s, 'info': %s }", vc->name, vc->info_str);
> +        net_device = qobject_to_qdict(obj);
> +
> +        if (vc->peer)
> +            qdict_put(net_device, "peer", qstring_from_str(vc->peer->name));
> +
> +        qlist_append(device_list, net_device);
>      }
> +
> +    *ret_data = QOBJECT(device_list);

 So, this returns a list of all devices, each of them is a qdict. If
the device is on vlan, it will have a vlan key.

 This is simple and looks ok to me.

>  }
>  
>  void do_set_link(Monitor *mon, const QDict *qdict)
> diff --git a/net.h b/net.h
> index 16f19c5..f16c2d6 100644
> --- a/net.h
> +++ b/net.h
> @@ -6,6 +6,7 @@
>  #include "qemu-common.h"
>  #include "qdict.h"
>  #include "qemu-option.h"
> +#include "qobject.h"
>  #include "net/queue.h"
>  
>  struct MACAddr {
> @@ -117,7 +118,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);
>  void do_set_link(Monitor *mon, const QDict *qdict);
>  
>  /* NIC info */