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

[Jan Kiszka]

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).

> +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, "   \\ ");

Two space too much of indention (you remove them above for the existing
peer \ peer outputs).

> +            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