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

[Michael Roth]

Quoting address@hidden (2017-10-25 20:42:10)
> 
> >> +    if (NO_ERROR == GetIfEntry(&aMib_ifrow)) {
> 
> 
> thanks,
> 
> I found may be overflow using GetIfEntry,thus GetIfEntry2 instead of 
> GetIfEntry.

I tried your fix-up patch at:
 https://patch-diff.githubusercontent.com/raw/mdroth/qemu/pull/5.patch

but ran into the compile issue below in a Fedora 20 mingw64 build environment.
It looks like you need MIB_IF_ROW2 instead of MIB_IFROW2, and even then it
looks like the struct fields are different and need to be fixed up.

I'll keep the current patch as it stands, but please send a tested
standalone fix against the qga tree to the list so we can get this case
addressed before rc0. I would also suggest we disable w32 stats completely
if GetIfEntry2() isn't available, since dealing with overflows is painful
from a management perspective.


x86_64-w64-mingw32-gcc -I/home/mdroth/qemu-build-w64/qga -Iqga 
-I/home/mdroth/w/qemu4.git/tcg -I/home/mdroth/w/qemu4.git/tcg/i386 -I. 
-I/home/mdroth/w/qemu4.git -I/home/mdroth/w/qemu4.git/accel/tcg 
-I/home/mdroth/w/qemu4.git/include 
-I/usr/x86_64-w64-mingw32/sys-root/mingw/include/pixman-1  
-I/home/mdroth/w/qemu4.git/dtc/libfdt -Werror -mms-bitfields 
-I/usr/x86_64-w64-mingw32/sys-root/mingw/include/glib-2.0 
-I/usr/x86_64-w64-mingw32/sys-root/mingw/lib/glib-2.0/include  -m64 -mcx16 
-mthreads -D__USE_MINGW_ANSI_STDIO=1 -DWIN32_LEAN_AND_MEAN -DWINVER=0x501 
-D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes 
-Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes 
-fno-strict-aliasing -fno-common -fwrapv  -Wall -Wendif-labels 
-Wno-missing-include-dirs -Wempty-body -Wnested-externs -Wformat-security 
-Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-declaration 
-Wold-style-definition -Wtype-limits -fstack-protector-all -isystem 
/home/mdroth/w/vss-win32/ -I/home/mdroth/w/qemu4.git/tests -I 
qga/qapi-generated -MMD -MP -MT qga/service-win32.o -MF qga/service-win32.d -O2 
-U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -g   -c -o qga/service-win32.o 
/home/mdroth/w/qemu4.git/qga/service-win32.c
/home/mdroth/w/qemu4.git/qga/commands-win32.c: In function 
'guest_get_network_stats':
/home/mdroth/w/qemu4.git/qga/commands-win32.c:1200:20: error: 'MIB_IF_ROW2' has 
no member named 'dwIndex'
         a_mid_ifrow.dwIndex = if_index;
                    ^
/home/mdroth/w/qemu4.git/qga/commands-win32.c:1202:42: error: 'MIB_IF_ROW2' has 
no member named 'dwInOctets'
             stats->rx_bytes = a_mid_ifrow.dwInOctets;
                                          ^
/home/mdroth/w/qemu4.git/qga/commands-win32.c:1203:44: error: 'MIB_IF_ROW2' has 
no member named 'dwInUcastPkts'
             stats->rx_packets = a_mid_ifrow.dwInUcastPkts;
                                            ^
/home/mdroth/w/qemu4.git/qga/commands-win32.c:1204:41: error: 'MIB_IF_ROW2' has 
no member named 'dwInErrors'
             stats->rx_errs = a_mid_ifrow.dwInErrors;
                                         ^
/home/mdroth/w/qemu4.git/qga/commands-win32.c:1205:44: error: 'MIB_IF_ROW2' has 
no member named 'dwInDiscards'
             stats->rx_dropped = a_mid_ifrow.dwInDiscards;
                                            ^
/home/mdroth/w/qemu4.git/qga/commands-win32.c:1206:42: error: 'MIB_IF_ROW2' has 
no member named 'dwOutOctets'
             stats->tx_bytes = a_mid_ifrow.dwOutOctets;
                                          ^
/home/mdroth/w/qemu4.git/qga/commands-win32.c:1207:44: error: 'MIB_IF_ROW2' has 
no member named 'dwOutUcastPkts'
             stats->tx_packets = a_mid_ifrow.dwOutUcastPkts;
                                            ^
/home/mdroth/w/qemu4.git/qga/commands-win32.c:1208:41: error: 'MIB_IF_ROW2' has 
no member named 'dwOutErrors'
             stats->tx_errs = a_mid_ifrow.dwOutErrors;
                                         ^
/home/mdroth/w/qemu4.git/qga/commands-win32.c:1209:44: error: 'MIB_IF_ROW2' has 
no member named 'dwOutDiscards'
             stats->tx_dropped = a_mid_ifrow.dwOutDiscards;
                                            ^
make: *** [qga/commands-win32.o] Error 1
make: *** Waiting for unfinished jobs....

> 
> 
> 为了让您的VPlat虚拟机故障和docker故障得到高效的处理，请上报故障到: $VPlat技术支
> 持。
> 
> 芦志朋 luzhipeng
> 
> 
> IT开发工程师 IT Development Engineer
> 操作系统产品部/中心研究院/系统产品 OS Product Dept./Central R＆D Institute/
> System Product
> 
> 
> [cid]          [cid]
>                四川省成都市天府大道中段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
> > 
> 
>