Re: [Qemu-devel] [PATCH v4 13/16] net: Make "info network" output more readable info

[Zhi Yong Wu]

On Fri, May 25, 2012 at 10:17 PM, Jan Kiszka <address@hidden> wrote:
> On 2012-05-25 11:02, address@hidden wrote:
>> From: Zhi Yong Wu <address@hidden>
>>
>> Reviewed-by:   Jan Kiszka  <address@hidden>
>
> Please don't put reviewed-by tags here before the reviewer had a chance
> to look at the code.
>
>> Signed-off-by: Zhi Yong Wu <address@hidden>
>> ---
>>  net.c     |   18 ++++++++++++++----
>>  net.h     |   12 ++++++++++++
>>  net/hub.c |   23 +++++++++++++++++++++--
>>  net/hub.h |    1 +
>>  4 files changed, 48 insertions(+), 6 deletions(-)
>>
>> diff --git a/net.c b/net.c
>> index 61dc28d..ae0deec 100644
>> --- a/net.c
>> +++ b/net.c
>> @@ -887,6 +887,12 @@ static const struct {
>>          },
>>      },
>>  #endif /* CONFIG_NET_BRIDGE */
>> +    [NET_CLIENT_TYPE_HUB] = {
>> +        .type = "hubport",
>> +        .desc = {
>> +            { /* end of list */ }
>> +        },
>> +    },
>
> This should be obsolete now.
>
>>  };
>>
>>  int net_client_init(Monitor *mon, QemuOpts *opts, int is_netdev)
>> @@ -1068,7 +1074,7 @@ int do_netdev_del(Monitor *mon, const QDict *qdict, 
>> QObject **ret_data)
>>      return 0;
>>  }
>>
>> -static void print_net_client(Monitor *mon, NetClientState *vc)
>> +void print_net_client(Monitor *mon, NetClientState *vc)
>>  {
>>      monitor_printf(mon, "%s: type=%s,%s\n", vc->name,
>>                     net_client_types[vc->info->type].type, vc->info_str);
>> @@ -1079,12 +1085,17 @@ void do_info_network(Monitor *mon)
>>      NetClientState *nc, *peer;
>>      net_client_type type;
>>
>> -    monitor_printf(mon, "Devices not on any VLAN:\n");
>> +    net_hub_info(mon);
>> +
>>      QTAILQ_FOREACH(nc, &net_clients, next) {
>>          peer = nc->peer;
>>          type = nc->info->type;
>> +
>> +        if (net_hub_port_peer_nc(nc)) {
>> +            continue;
>> +        }
>> +
>>          if (!peer || type == NET_CLIENT_TYPE_NIC) {
>> -            monitor_printf(mon, "  ");
>>              print_net_client(mon, nc);
>>          } /* else it's a netdev connected to a NIC, printed with the NIC */
>>          if (peer && type == NET_CLIENT_TYPE_NIC) {
>> @@ -1092,7 +1103,6 @@ void do_info_network(Monitor *mon)
>>              print_net_client(mon, peer);
>>          }
>>      }
>> -    net_hub_info(mon);
>
> Why introduce a different hub output format at all? Do it in the final
> right from the start.
>
>>  }
>>
>>  void qmp_set_link(const char *name, bool up, Error **errp)
>> diff --git a/net.h b/net.h
>> index 250669a..08306a4 100644
>> --- a/net.h
>> +++ b/net.h
>> @@ -112,6 +112,18 @@ 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);
>>
>> +ssize_t qemu_deliver_packet(NetClientState *sender,
>> +                            unsigned flags,
>> +                            const uint8_t *data,
>> +                            size_t size,
>> +                            void *opaque);
>> +ssize_t qemu_deliver_packet_iov(NetClientState *sender,
>> +                            unsigned flags,
>> +                            const struct iovec *iov,
>> +                            int iovcnt,
>> +                            void *opaque);
>> +
>
> I bet those two prototypes are required by some other patch (or are even
> redundant).
I have noticed this, and split out them to other patch.
>
>> +void print_net_client(Monitor *mon, NetClientState *vc);
>>  void do_info_network(Monitor *mon);
>>
>>  /* NIC info */
>> diff --git a/net/hub.c b/net/hub.c
>> index 122de69..8c77d03 100644
>> --- a/net/hub.c
>> +++ b/net/hub.c
>> @@ -184,6 +184,25 @@ NetClientState *net_hub_find_client_by_name(unsigned 
>> int hub_id,
>>  }
>>
>>  /**
>> + * Determine if one nc peers with one hub port
>> + */
>> +bool net_hub_port_peer_nc(NetClientState *nc)
>> +{
>> +    NetHub *hub;
>> +    NetHubPort *port;
>> +
>> +    QLIST_FOREACH(hub, &hubs, next) {
>> +        QLIST_FOREACH(port, &hub->ports, next) {
>> +            if (nc == port->nc.peer) {
>> +                return true;
>> +            }
>> +        }
>> +    }
>> +
>> +    return false;
>> +}
>> +
>> +/**
>>   * Print hub configuration
>>   */
>>  void net_hub_info(Monitor *mon)
>> @@ -194,8 +213,8 @@ void net_hub_info(Monitor *mon)
>>      QLIST_FOREACH(hub, &hubs, next) {
>>          monitor_printf(mon, "hub %u\n", hub->id);
>>          QLIST_FOREACH(port, &hub->ports, next) {
>> -            monitor_printf(mon, "    port %u peer %s\n", port->id,
>> -                           port->nc.peer ? port->nc.peer->name : "<none>");
>> +            monitor_printf(mon, "   \\ ");
This will have the following layout:
(qemu) info network
hub 1
   \ dump.0: type=dump,dump to qemu-vlan1.pcap (len=65536)
   \ user.1: type=user,net=10.0.2.0,restrict=off
   \ virtio-net-pci.0: type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:58
hub 0
   \ user.0: type=user,net=10.0.2.0,restrict=off
   \ e1000.0: type=nic,model=e1000,macaddr=52:54:00:12:34:57
virtio-net-pci.1: type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:56
   \ ur: type=tap,ifname=tap0,script=/etc/qemu-ifup,downscript=/etc/qemu-ifdown
^^^
>
> Two space too much of indention (you remove them above for the existing
It is three space, not two.
> peer \ peer outputs).
Do you mean that it should be like "monitor_printf(mon, "\\ ");" but
this indention will not match with those peers in hub case.

>
>> +            print_net_client(mon, port->nc.peer);
>>          }
>>      }
>>  }
>> diff --git a/net/hub.h b/net/hub.h
>> index ff5024a..550189b 100644
>> --- a/net/hub.h
>> +++ b/net/hub.h
>> @@ -23,5 +23,6 @@ NetClientState *net_hub_find_client_by_name(unsigned int 
>> hub_id,
>>  void net_hub_info(Monitor *mon);
>>  int net_hub_id_for_client(NetClientState *nc, unsigned int *id);
>>  void net_hub_check_clients(void);
>> +bool net_hub_port_peer_nc(NetClientState *nc);
>>
>>  #endif /* NET_HUB_H */
>
> Jan
>
> --
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux



-- 
Regards,

Zhi Yong Wu