[Qemu-devel] 答复: Re: [PATCH v7 RESEND] qga: Add support network interface statistics inguest-network-get-interfaces command

[lu.zhipeng]

>> +    if (NO_ERROR == GetIfEntry(&aMib_ifrow)) {






thanks,


I found may be overflow using GetIfEntry,thus GetIfEntry2 instead of GetIfEntry.











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


芦志朋 luzhipeng






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









四川省成都市天府大道中段800号
E: address@hidden 
www.zte.com.cn










原始邮件




发件人： <address@hidden>;
收件人：芦志朋10108272;
抄送人： <address@hidden>;芦志朋10108272;
日 期 ：2017年10月26日 09:00
主 题 ：Re: [PATCH v7 RESEND] qga: Add support network interface statistics 
inguest-network-get-interfaces command






Quoting ZhiPeng Lu (2017-09-12 03:54:26)
> 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.
> 
> Signed-off-by: ZhiPeng Lu <address@hidden>

Thanks, applied to qga tree:
  https://github.com/mdroth/qemu/commits/qga

There were a few issues that I noted below, but I tested on linux/windows
and things seem to be working well so I fixed them up in my tree.

> 
> ---
> v1->v2:
>  - correct some spelling mistake and add the stats data to the
>    guest-network-get-interfaces command instead of adding a new command.
> v2-v3:
>  - optimize function implementation
> v3->v4:
>  - modify compile error
> v4->v5:
>  - rename some temporary variables and add str_trim_off function for
>    calculating the space num in front of the string in guest_get_network_stats
> v5->v6:
>  - use g_strchug instead of str_trim_off implemented by myself
> v6->v7:
>  - add implementation for windows
> ---
>  qga/commands-posix.c | 72 
> +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  qga/commands-win32.c | 48 +++++++++++++++++++++++++++++++++++
>  qga/qapi-schema.json | 38 ++++++++++++++++++++++++++-
>  3 files changed, 156 insertions(+), 2 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index ab0c63d..da5dba0 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -1643,6 +1643,65 @@ guest_find_interface(GuestNetworkInterfaceList *head,
>      return head;
>  }
> 
> +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;

Not sure if it affects behavior in practice, but getline() documentation
states that *line is only allocated if *line == NULL *and* n == 0. So we
should set n to 0 before each call to be safe.

> +    fp = fopen(devinfo, "r");
> +    if (!fp) {
> +        return -1;
> +    }
> +    name_len = strlen(name);
> +    while (getline(&line, &n, fp) != -1) {

Since 'line' is being allocated by getline(), we need to free it after
each call, or rely on it to realloc and then free that value before
returning. The latter seems simpler in this case.

> +        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;
> +        char *trim_line;
> +        trim_line = g_strchug(line);
> +        if (trim_line[0] == '\0') {
> +            continue;
> +        }
> +        colon = strchr(trim_line, ':');
> +        if (!colon) {
> +            continue;
> +        }
> +        if (colon - name_len  == trim_line &&
> +           strncmp(trim_line, name, name_len) == 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");

Reporting the specific interface name might be useful here.

> +    return -1;
> +}
> +
>  /*
>   * Build information about guest interfaces
>   */
> @@ -1659,6 +1718,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;
> @@ -1778,7 +1838,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/commands-win32.c b/qga/commands-win32.c
> index 619dbd2..e891253 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -1152,6 +1152,42 @@ out:
>  }
>  #endif
> 
> +static DWORD get_interface_index(const char *guid)
> +{
> +    ULONG index;
> +    DWORD status;
> +    wchar_t wbuf[512];
> +    snwprintf(wbuf, sizeof(wbuf), L"\\device\\tcpip_%s", guid);
> +    wbuf[sizeof(wbuf) - 1] = 0;

wchar_t can be larger than 1 byte, so using sizeof(wchar_t[]) as an
index into a wchar_t[] can cause an overrun.

> +    status = GetAdapterIndex (wbuf, &index);
> +    if (status != NO_ERROR) {
> +        return (DWORD)~0;
> +    } else {
> +        return index;
> +    }
> +}
> +static int guest_get_network_stats(const char *name,
> +                       GuestNetworkInterfaceStat *stats)
> +{
> +    DWORD IfIndex = 0;
> +    MIB_IFROW aMib_ifrow;
> +    memset(&aMib_ifrow, 0, sizeof(aMib_ifrow));
> +    IfIndex = get_interface_index(name);

Per CODING_STYLE variable names should be lowercase with "_" for
separating words

> +    aMib_ifrow.dwIndex = IfIndex;
> +    if (NO_ERROR == GetIfEntry(&aMib_ifrow)) {
> +        stats->rx_bytes = aMib_ifrow.dwInOctets;
> +        stats->rx_packets = aMib_ifrow.dwInUcastPkts;
> +        stats->rx_errs = aMib_ifrow.dwInErrors;
> +        stats->rx_dropped = aMib_ifrow.dwInDiscards;
> +        stats->tx_bytes = aMib_ifrow.dwOutOctets;
> +        stats->tx_packets = aMib_ifrow.dwOutUcastPkts;
> +        stats->tx_errs = aMib_ifrow.dwOutErrors;
> +        stats->tx_dropped = aMib_ifrow.dwOutDiscards;
> +        return 0;
> +    }
> +    return -1;
> +}
> +
>  GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
>  {
>      IP_ADAPTER_ADDRESSES *adptr_addrs, *addr;
> @@ -1159,6 +1195,7 @@ GuestNetworkInterfaceList 
> *qmp_guest_network_get_interfaces(Error **errp)
>      GuestNetworkInterfaceList *head = NULL, *cur_item = NULL;
>      GuestIpAddressList *head_addr, *cur_addr;
>      GuestNetworkInterfaceList *info;
> +    GuestNetworkInterfaceStat *interface_stat = NULL;
>      GuestIpAddressList *address_item = NULL;
>      unsigned char *mac_addr;
>      char *addr_str;
> @@ -1238,6 +1275,17 @@ GuestNetworkInterfaceList 
> *qmp_guest_network_get_interfaces(Error **errp)
>              info->value->has_ip_addresses = true;
>              info->value->ip_addresses = head_addr;
>          }
> +        if (!info->value->has_statistics) {
> +            interface_stat = g_malloc0(sizeof(*interface_stat));
> +            if (guest_get_network_stats(addr->AdapterName,
> +                interface_stat) == -1) {
> +                info->value->has_statistics = false;
> +                g_free(interface_stat);
> +            } else {
> +                info->value->statistics = interface_stat;
> +                info->value->has_statistics = true;
> +            }
> +        }
>      }
>      WSACleanup();
>  out:
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index 90a0c86..17884c7 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -643,6 +643,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.11
> +##
> +{ '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
> @@ -651,12 +683,16 @@
>  #
>  # @ip-addresses: List of addresses assigned to @name
>  #
> +# @statistics: various statistic counters related to @name
> +# (since 2.11)
> +#
>  # 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
>