[Qemu-devel] 答复: Re: [PATCH RESEND v6] qga: Add support networkinterface statistics in guest-network-get-interfaces command

[lu.zhipeng]

Thanks for the review.




>Hi>On Tue, Jul 4, 2017 at 10:51 AM ZhiPeng Lu <address@hidden> wrote:> >we can 
>get the network interface statistics inside a virtual machine by> 
>>guest-network-get-interfaces command. it is very useful for us to monitor> 
>>and analyze network traffic.>>>>


>It's nicer if you give v1->v2->..->v6 change summary at each revision

it's a good  suggestion.




>> Signed-off-by: ZhiPeng Lu <address@hidden>>>--->>  qga/commands-posix.c | 
>> 80>> +++++++++++++++++++++++++++++++++++++++++++++++++++->>  
>> qga/qapi-schema.json | 38 ++++++++++++++++++++++++->>  2 files changed, 116 
>> insertions(+), 2 deletions(-)>>>> diff --git a/qga/commands-posix.c 
>> b/qga/commands-posix.c>> index 915df9e..233b024 100644>> --- 
>> a/qga/commands-posix.c>> +++ b/qga/commands-posix.c>> @@ -1638,6 +1638,73 @@ 
>> guest_find_interface(GuestNetworkInterfaceList>> *head,>>      return head>> 
>>  }>>>> +>> +static int str_trim_off(const char *s, int off, int lmt)>> +{>> 
>> +    for ( off < lmt ++off) {>> +        if (!isspace(s[off])) {>> +         
>>    break>> +        }>> +    }>> +    return off>> +}>> +>> +static int 
>> guest_get_network_stats(const char *name,>> +                       
>> GuestNetworkInterfaceStat *stats)>> +{>> +    int name_len>> +    char const 
>> *devinfo = "/proc/net/dev">> +    FILE *fp>> +    char *line = NULL, 
>> *colon>> +    size_t n>> +    fp = fopen(devinfo, "r")>> +    if (!fp) {>>+  
>>       return -1>> +    }>> +    name_len = strlen(name)>> +    while 
>> (getline(&line, &n, fp) != -1) {>> +        long long dummy>> +        long 
>> long rx_bytes>> +        long long rx_packets>> +        long long rx_errs>> 
>> +        long long rx_dropped>> +        long long tx_bytes>> +        long 
>> long tx_packets>> +        long long tx_errs>> +        long long 
>> tx_dropped>>> Why have local variables, and not pass stats->filed directly?

if passing stats->filed directly ,stats->filed stores wrong data when it 
doesn't  find the network interface.


>>+        int trim_off>>+        colon = strchr(line, ':')>> +        if 
>>(!colon) {>> +            continue>> +        }>> +        trim_off = 
>>str_trim_off(line, 0, strlen(line))>>>Couldn't you  call g_strchomp() ?







i agreed,i will call g_strchomp().

>> +        if (colon - name_len - trim_off == line &&>>+           
>> strncmp(line + trim_off, name, colon - line - trim_off) == 0) {>> +          
>>   if (sscanf(colon + 1,>> +                "%lld %lld %lld %lld %lld %lld 
>> %lld %lld %lld %lld %lld>> %lld %lld %lld %lld %lld",>> +                  
>> &rx_bytes, &rx_packets, &rx_errs, &rx_dropped,>> +                  &dummy, 
>> &dummy, &dummy, &dummy,>> +                  &tx_bytes, &tx_packets, 
>> &tx_errs, &tx_dropped,>> +                  &dummy, &dummy, &dummy, &dummy) 
>> != 16) {>> +                continue>> +            }>> +            
>> stats->rx_bytes = rx_bytes>>+            stats->rx_packets = rx_packets>> +  
>>          stats->rx_errs = rx_errs>> +            stats->rx_dropped = 
>> rx_dropped>> +            stats->tx_bytes = tx_bytes>> +            
>> stats->tx_packets = tx_packets>> +            stats->tx_errs = tx_errs>> +   
>>          stats->tx_dropped = tx_dropped>> +            fclose(fp)>> +        
>>     return 0>> +        }>> +    }>> +    fclose(fp)>> +    
>> g_debug("/proc/net/dev: Interface not found")>> +    return -1>> +}>> +>>  
>> /*>>   * Build information about guest interfaces>>  */>> @@ -1654,6 +1721,7 
>> @@ GuestNetworkInterfaceList>> *qmp_guest_network_get_interfaces(Error 
>> **errp)>>      for (ifa = ifap ifa ifa = ifa->ifa_next) {> >         
>> GuestNetworkInterfaceList *info> >         GuestIpAddressList **address_list 
>> = NULL, *address_item = NULL>> +        GuestNetworkInterfaceStat  
>> *interface_stat = NULL>>          char addr4[INET_ADDRSTRLEN]>>          
>> char addr6[INET6_ADDRSTRLEN]>>          int sock>> @@ -1773,7 +1841,17 @@ 
>> GuestNetworkInterfaceList>> *qmp_guest_network_get_interfaces(Error 
>> **errp)>>>>          info->value->has_ip_addresses = true>>>> ->> +        
>> if (!info->value->has_statistics) {>> +            interface_stat = 
>> g_malloc0(sizeof(*interface_stat))>> +            if 
>> (guest_get_network_stats(info->value->name,>> +                
>> interface_stat) == -1) {>> +                info->value->has_statistics = 
>> false>> +                g_free(interface_stat)>> +            } else {>> +  
>>               info->value->statistics = interface_stat>> +                
>> info->value->has_statistics = true>> +            }>> +        }>>      
>> }>>>>      freeifaddrs(ifap)>> diff --git a/qga/qapi-schema.json 
>> b/qga/qapi-schema.json>> index a02dbf2..948219b 100644>> --- 
>> a/qga/qapi-schema.json>> +++ b/qga/qapi-schema.json>> @@ -635,6 +635,38 @@>> 
>>             'prefix': 'int'} }>>>>  ##>> +# @GuestNetworkInterfaceStat:>> 
>> +#>> +# @rx-bytes: total bytes received>> +#>> +# @rx-packets: total packets 
>> received>> +#>> +# @rx-errs: bad packets received>> +#>> +# @rx-dropped: 
>> receiver dropped packets>> +#>> +# @tx-bytes: total bytes transmitted>> +#>> 
>> +# @tx-packets: total packets transmitted>> +#>> +# @tx-errs: packet 
>> transmit problems>> +#>> +# @tx-dropped: dropped packets transmitted>> +#>> 
>> +# Since: 2.10>> +##>> +{ 'struct': 'GuestNetworkInterfaceStat',>> +  
>> 'data': {'rx-bytes': 'uint64',>> +            'rx-packets': 'uint64',>> +    
>>         'rx-errs': 'uint64',>> +            'rx-dropped': 'uint64',>> +      
>>       'tx-bytes': 'uint64',>> +            'tx-packets': 'uint64',>> +       
>>      'tx-errs': 'uint64',>> +            'tx-dropped': 'uint64'>> +          
>>  } }>> +>> +##>>  # @GuestNetworkInterface:>>  #>>  # @name: The name of 
>> interface for which info are being delivered>> @@ -643,12 +675,16 @@>>  #>>  
>> # @ip-addresses: List of addresses assigned to @name>>  #>> +# @statistics: 
>> various statistic counters related to @name>> +# (since 2.10)>> +#>>  # 
>> Since: 1.1>>  ##>>  { 'struct': 'GuestNetworkInterface',>>    'data': 
>> {'name': 'str',>>             '*hardware-address': 'str',>> -           
>> '*ip-addresses': ['GuestIpAddress'] } }>> +           '*ip-addresses': 
>> ['GuestIpAddress'],>> +           '*statistics': 'GuestNetworkInterfaceStat' 
>> } }>>>>  ##>>  # @guest-network-get-interfaces:>> -->> 1.8.3.1>>>looks good 
>> to me otherwise,>-- >Marc-André Lureau



















为了让您的VPlat虚拟化故障得到高效的处理，请上报故障到: $VPlat技术支持。


芦志朋 luzhipeng






IT开发工程师 IT Development
Engineer
操作系统产品部/中心研究院/系统产品 OS Product Dept./Central R＆D Institute/System Product









深圳市南山区科技南路55号中兴通讯研发大楼33楼 
33/F, R&D Building, ZTE
Corporation Hi-tech Road South, 
Hi-tech
Industrial Park Nanshan District, Shenzhen, P.R.China, 518057 
T: +86 755 xxxxxxxx F:+86 755 xxxxxxxx 
M: +86 xxxxxxxxxxx 
E: address@hidden 
www.zte.com.cn






原始邮件



发件人： <address@hidden>
收件人：芦志朋10108272 <address@hidden>
抄送人： <address@hidden>
日 期 ：2017年07月04日 22:33
主 题 ：Re: [Qemu-devel] [PATCH RESEND v6] qga: Add support networkinterface 
statistics in guest-network-get-interfaces command





Hi

On Tue, Jul 4, 2017 at 10:51 AM ZhiPeng Lu <address@hidden> wrote:

> we can get the network interface statistics inside a virtual machine by
> guest-network-get-interfaces command. it is very useful for us to monitor
> and analyze network traffic.
>
>
It's nicer if you give v1->v2->..->v6 change summary at each revision


> Signed-off-by: ZhiPeng Lu <address@hidden>
> ---
>  qga/commands-posix.c | 80
> +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  qga/qapi-schema.json | 38 ++++++++++++++++++++++++-
>  2 files changed, 116 insertions(+), 2 deletions(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 915df9e..233b024 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -1638,6 +1638,73 @@ guest_find_interface(GuestNetworkInterfaceList
> *head,
>      return head
>  }
>
> +
> +static int str_trim_off(const char *s, int off, int lmt)
> +{
> +    for ( off < lmt ++off) {
> +        if (!isspace(s[off])) {
> +            break
> +        }
> +    }
> +    return off
> +}
> +
> +static int guest_get_network_stats(const char *name,
> +                       GuestNetworkInterfaceStat *stats)
> +{
> +    int name_len
> +    char const *devinfo = "/proc/net/dev"
> +    FILE *fp
> +    char *line = NULL, *colon
> +    size_t n
> +    fp = fopen(devinfo, "r")
> +    if (!fp) {
> +        return -1
> +    }
> +    name_len = strlen(name)
> +    while (getline(&line, &n, fp) != -1) {
> +        long long dummy
> +        long long rx_bytes
> +        long long rx_packets
> +        long long rx_errs
> +        long long rx_dropped
> +        long long tx_bytes
> +        long long tx_packets
> +        long long tx_errs
> +        long long tx_dropped
>

 Why have local variables, and not pass stats->filed directly?

+        int trim_off
> +        colon = strchr(line, ':')
> +        if (!colon) {
> +            continue
> +        }
> +        trim_off = str_trim_off(line, 0, strlen(line))
>

Couldn't you  call g_strchomp() ?


> +        if (colon - name_len - trim_off == line &&
> +           strncmp(line + trim_off, name, colon - line - trim_off) == 0) {
> +            if (sscanf(colon + 1,
> +                "%lld %lld %lld %lld %lld %lld %lld %lld %lld %lld %lld
> %lld %lld %lld %lld %lld",
> +                  &rx_bytes, &rx_packets, &rx_errs, &rx_dropped,
> +                  &dummy, &dummy, &dummy, &dummy,
> +                  &tx_bytes, &tx_packets, &tx_errs, &tx_dropped,
> +                  &dummy, &dummy, &dummy, &dummy) != 16) {
> +                continue
> +            }
> +            stats->rx_bytes = rx_bytes
> +            stats->rx_packets = rx_packets
> +            stats->rx_errs = rx_errs
> +            stats->rx_dropped = rx_dropped
> +            stats->tx_bytes = tx_bytes
> +            stats->tx_packets = tx_packets
> +            stats->tx_errs = tx_errs
> +            stats->tx_dropped = tx_dropped
> +            fclose(fp)
> +            return 0
> +        }
> +    }
> +    fclose(fp)
> +    g_debug("/proc/net/dev: Interface not found")
> +    return -1
> +}
> +
>  /*
>   * Build information about guest interfaces
>   */
> @@ -1654,6 +1721,7 @@ GuestNetworkInterfaceList
> *qmp_guest_network_get_interfaces(Error **errp)
>      for (ifa = ifap ifa ifa = ifa->ifa_next) {
>          GuestNetworkInterfaceList *info
>          GuestIpAddressList **address_list = NULL, *address_item = NULL
> +        GuestNetworkInterfaceStat  *interface_stat = NULL
>          char addr4[INET_ADDRSTRLEN]
>          char addr6[INET6_ADDRSTRLEN]
>          int sock
> @@ -1773,7 +1841,17 @@ GuestNetworkInterfaceList
> *qmp_guest_network_get_interfaces(Error **errp)
>
>          info->value->has_ip_addresses = true
>
> -
> +        if (!info->value->has_statistics) {
> +            interface_stat = g_malloc0(sizeof(*interface_stat))
> +            if (guest_get_network_stats(info->value->name,
> +                interface_stat) == -1) {
> +                info->value->has_statistics = false
> +                g_free(interface_stat)
> +            } else {
> +                info->value->statistics = interface_stat
> +                info->value->has_statistics = true
> +            }
> +        }
>      }
>
>      freeifaddrs(ifap)
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index a02dbf2..948219b 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -635,6 +635,38 @@
>             'prefix': 'int'} }
>
>  ##
> +# @GuestNetworkInterfaceStat:
> +#
> +# @rx-bytes: total bytes received
> +#
> +# @rx-packets: total packets received
> +#
> +# @rx-errs: bad packets received
> +#
> +# @rx-dropped: receiver dropped packets
> +#
> +# @tx-bytes: total bytes transmitted
> +#
> +# @tx-packets: total packets transmitted
> +#
> +# @tx-errs: packet transmit problems
> +#
> +# @tx-dropped: dropped packets transmitted
> +#
> +# Since: 2.10
> +##
> +{ 'struct': 'GuestNetworkInterfaceStat',
> +  'data': {'rx-bytes': 'uint64',
> +            'rx-packets': 'uint64',
> +            'rx-errs': 'uint64',
> +            'rx-dropped': 'uint64',
> +            'tx-bytes': 'uint64',
> +            'tx-packets': 'uint64',
> +            'tx-errs': 'uint64',
> +            'tx-dropped': 'uint64'
> +           } }
> +
> +##
>  # @GuestNetworkInterface:
>  #
>  # @name: The name of interface for which info are being delivered
> @@ -643,12 +675,16 @@
>  #
>  # @ip-addresses: List of addresses assigned to @name
>  #
> +# @statistics: various statistic counters related to @name
> +# (since 2.10)
> +#
>  # Since: 1.1
>  ##
>  { 'struct': 'GuestNetworkInterface',
>    'data': {'name': 'str',
>             '*hardware-address': 'str',
> -           '*ip-addresses': ['GuestIpAddress'] } }
> +           '*ip-addresses': ['GuestIpAddress'],
> +           '*statistics': 'GuestNetworkInterfaceStat' } }
>
>  ##
>  # @guest-network-get-interfaces:
> --
> 1.8.3.1
>

looks good to me otherwise,

-- 
Marc-André Lureau