Re: [Qemu-devel] 答复: Re: 答复: Re: [PATCH v2] qga: replace GetIfEntry

[Michael Roth]

Quoting address@hidden (2017-11-14 05:09:35)
>  i test the latest qga in xp , it run ok .
> 
> 
> my qga config :
> 
> Configured with: './configure' '--enable-guest-agent' '--cross-prefix=
> x86_64-w64-mingw32-' '--with-vss-sdk=/home/VSSSDK72' '--disable-fdt'        
> '--target-list=x86_64-softmmu'

Hmm, so you're testing with Windows XP x64? I was using XP 32-bit (SP3),
but I retried with XP x64 (SP2) and I still have the same issue.

I can only get qemu-ga working if I build on top of something prior to
commit 12f8def0e.

What build environment are you using? I've tried Fedora Core 18 and 20
and have the same issue with both.

> 
> used qga version info
> 
> address@hidden qemu]# git log
> 
> commit 533ab83ea074d5fc457769f6ac698524a12f1156
> 
> Author: ZhiPeng Lu <address@hidden>
> 
> Date:   Fri Nov 10 10:17:14 2017 +0800
> 
> 
>     qga: fix some errors for guest_get_network_stats
> 
>     
> 
>     fix some erros:
> 
>     1.if building qga on Windows Vista/2008 and newer,
> 
>     it cann't find the link to GetIfEntry2 in windows xp.
> 
>     2. check valid of if_index.
> 
>     
> 
>     Signed-off-by: ZhiPeng Lu <address@hidden>
> 
> 
> commit de597a8b27722ce4f9cc660f930f7dccc712712d
> 
> Author: ZhiPeng Lu <address@hidden>
> 
> Date:   Fri Nov 3 22:54:20 2017 +0800
> 
> 
>     qga: replace GetIfEntry
> 
>     
> 
>     The data obtained by GetIfEntry is 32 bits, and it may overflow. Thus 
> using
> GetIfEntry2 instead of GetIfEntry.
> 
>     
> 
>     Signed-off-by: ZhiPeng Lu <address@hidden>
> 
>     *avoid CamelCase variable names
> 
>     *update field names for MIB_IFROW -> MIB_IF_ROW2
> 
>     Signed-off-by: Michael Roth <address@hidden>
> 
> 
> commit 5ca7a3cba468736cfe555887af1f6ba754f6eac9
> 
> Merge: a4f0537 10a7b7e
> 
> Author: Peter Maydell <address@hidden>
> 
> Date:   Tue Nov 7 14:43:35 2017 +0000
> 
> 
>     Merge remote-tracking branch 'remotes/berrange/tags/pull-2017-11-06-2' 
> into
> staging
> 
>     
> 
>     Pull IO 2017/11/06 v2
> 
> 
> 
> 
> 为了让您的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>;
> 日期：2017年11月14日 07:57
> 主题：Re: 答复: Re: [PATCH v2] qga: replace GetIfEntry
> Quoting address@hidden (2017-11-09 05:26:15)
> >  i think the code is better
> > 
> >  if (OSver.dwMajorVersion >= 6) {
> >       MIB_IF_ROW2 aMib_ifrow;
> >       typedef NETIOAPI_API (WINAPI *getifentry2_t)(PMIB_IF_ROW2 Row);
> >       memset(&aMib_ifrow, 0, sizeof(aMib_ifrow));
> >       aMib_ifrow.InterfaceIndex = nicId;
> >       HMODULE module = GetModuleHandle("iphlpapi");
> >       PVOID fun = GetProcAddress(module, "GetIfEntry2");
> >       if (fun == NULL) {
> >           error_setg(errp, QERR_QGA_COMMAND_FAILED,
> >                      "Failed to get address of GetIfEntry2");
> >           return NULL;
> >       }
> >     getifentry2_t getifentry2_ex = (getifentry2_t)fun;
> >     if (NO_ERROR == getifentry2_ex(&aMib_ifrow)){
> >     }
> 
> I've updated the patch with this change:
>   https://github.com/mdroth/qemu/commits/qga-if-stats
> 
> But I'm a bit confused now: when I tried to test this on XP I realized that
> that qemu-ga no longer works on XP, and generates the following error
> when I try to start it (even without your stats patch):
> 
>   "The procedure entry point AcquireSRWLockExclusive could not be located
>    in the dynamic link library KERNEL32.dll"
> 
> I think this may be due to the following commit, which notes that Vista+
> are now required as a result:
> 
> commit 12f8def0e02232d7c6416ad9b66640f973c531d1
> Author: Andrey Shedel <address@hidden>
> Date:   Fri Mar 24 15:01:41 2017 -0700
> 
>     win32: replace custom mutex and condition variable with native
>     primitives
> 
> So, are you actually able to run on XP currently? If so, how? And if
> not, I think we have other issues that need to be addressed if we
> want to support XP still; I'm not even sure that's realistic at this
> point.
> 
> Unless there's actually a way to test QGA on XP right now I think I
> we should just get in the updated patch minus the dynamic DLL stuff,
> i.e.:
>   https://github.com/mdroth/qemu/commit/
> de597a8b27722ce4f9cc660f930f7dccc712712d
> 
> Make sense?
> 
> > 
> > 
> > 
> > 
> > 
> > 为了让您的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
> > 
> > 原始邮件
> > 发件人：芦志朋10108272
> > 收件人： <address@hidden>;
> > 抄送人： <address@hidden>;
> > 日期：2017年11月08日 10:54
> > 主题：答复: Re: [PATCH v2] qga: replace GetIfEntry
> > 
> > 
> > >I have the modified patch staged here:
> > 
> > >  https://github.com/mdroth/qemu/commit/
> > de597a8b27722ce4f9cc660f930f7dccc712712d
> > >Let me know if it looks okay and I'll send a pull request for rc1
> > 
> >  
> > 
> >
>  the code right, but running QGA on the Windows XP, error: can not find the 
> link
> > to  GetIfEntry2 in iphlpapi.DLL.
> > 
> > 
> > 
> > 
> > 为了让您的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年11月08日 01:07
> > 主题：Re: [PATCH v2] qga: replace GetIfEntry
> > 
> > 
> > Quoting ZhiPeng Lu (2017-11-03 09:54:20)
> > >
> >
>   The data obtained by GetIfEntry is 32 bits, and it may overflow. Thus using 
> GetIfEntry2 instead of GetIfEntry.
> > > 
> > > Signed-off-by: ZhiPeng Lu <address@hidden>
> > > ---
> > >  qga/commands-win32.c | 31 ++++++++++++++++++-------------
> > >  1 file changed, 18 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> > > index 0322188..d096dc2 100644
> > > --- a/qga/commands-win32.c
> > > +++ b/qga/commands-win32.c
> > > @@ -1173,20 +1173,25 @@ static int guest_get_network_stats
> (const char *name,
> > >                         GuestNetworkInterfaceStat *stats)
> > >  {
> > >      DWORD if_index = 0;
> > > -    MIB_IFROW a_mid_ifrow;
> > > -    memset(&a_mid_ifrow, 0, sizeof(a_mid_ifrow));
> > > +    OSVERSIONINFO OSver;
> > >      if_index = get_interface_index(name);
> > > -    a_mid_ifrow.dwIndex = if_index;
> > > -    if (NO_ERROR == GetIfEntry(&a_mid_ifrow)) {
> > > -        stats->rx_bytes = a_mid_ifrow.dwInOctets;
> > > -        stats->rx_packets = a_mid_ifrow.dwInUcastPkts;
> > > -        stats->rx_errs = a_mid_ifrow.dwInErrors;
> > > -        stats->rx_dropped = a_mid_ifrow.dwInDiscards;
> > > -        stats->tx_bytes = a_mid_ifrow.dwOutOctets;
> > > -        stats->tx_packets = a_mid_ifrow.dwOutUcastPkts;
> > > -        stats->tx_errs = a_mid_ifrow.dwOutErrors;
> > > -        stats->tx_dropped = a_mid_ifrow.dwOutDiscards;
> > > -        return 0;
> > > +    OSver.dwOSVersionInfoSize = sizeof(OSVERSIONINFO);
> > > +    GetVersionEx(&OSver);
> > > +    if (OSver.dwMajorVersion >= 6) {
> > > +        MIB_IF_ROW2 a_mid_ifrow;
> > > +        memset(&a_mid_ifrow, 0, sizeof(a_mid_ifrow));
> > > +        a_mid_ifrow.dwIndex = if_index;
> > > +        if (NO_ERROR == GetIfEntry2(&a_mid_ifrow)) {
> > > +            stats->rx_bytes = a_mid_ifrow.dwInOctets;
> > > +            stats->rx_packets = a_mid_ifrow.dwInUcastPkts;
> > > +            stats->rx_errs = a_mid_ifrow.dwInErrors;
> > > +            stats->rx_dropped = a_mid_ifrow.dwInDiscards;
> > > +            stats->tx_bytes = a_mid_ifrow.dwOutOctets;
> > > +            stats->tx_packets = a_mid_ifrow.dwOutUcastPkts;
> > > +            stats->tx_errs = a_mid_ifrow.dwOutErrors;
> > > +            stats->tx_dropped = a_mid_ifrow.dwOutDiscards;
> > > +            return 0;
> > 
> > This fails to build in my fc20 mingw64 environment:
> > 
> > /home/mdroth/w/qemu4.git/qga/
> > commands-win32.c:1184: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:1186: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:1187:44: error: 'MIB_IF_ROW2' has no member named 
> 'dwInUcastPkts'
> >              stats->rx_packets = a_mid_ifrow.dwInUcastPkts;
> > 
> > etc ...
> > 
> > The patch seems to assume MIB_IFROW and MIB_IF_ROW2 have the same field 
> > names
> > for the stats, but according to this they're slightly different:
> > 
> >   https://msdn.microsoft.com/en-us/library/windows/desktop/aa814491(v=
> > vs.85).aspx
> > 
> > This change on top of your patch seems to work:
> > 
> > diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> > index e17fc3c284..58e470877f 100644
> > --- a/qga/commands-win32.c
> > +++ b/qga/commands-win32.c
> > @@ -1181,16 +1181,16 @@ static int guest_get_network_stats(const char *name,
> >      if (os_ver.dwMajorVersion >= 6) {
> >          MIB_IF_ROW2 a_mid_ifrow;
> >          memset(&a_mid_ifrow, 0, sizeof(a_mid_ifrow));
> > -        a_mid_ifrow.dwIndex = if_index;
> > +        a_mid_ifrow.InterfaceIndex = if_index;
> >          if (NO_ERROR == GetIfEntry2(&a_mid_ifrow)) {
> > -            stats->rx_bytes = a_mid_ifrow.dwInOctets;
> > -            stats->rx_packets = a_mid_ifrow.dwInUcastPkts;
> > -            stats->rx_errs = a_mid_ifrow.dwInErrors;
> > -            stats->rx_dropped = a_mid_ifrow.dwInDiscards;
> > -            stats->tx_bytes = a_mid_ifrow.dwOutOctets;
> > -            stats->tx_packets = a_mid_ifrow.dwOutUcastPkts;
> > -            stats->tx_errs = a_mid_ifrow.dwOutErrors;
> > -            stats->tx_dropped = a_mid_ifrow.dwOutDiscards;
> > +            stats->rx_bytes = a_mid_ifrow.InOctets;
> > +            stats->rx_packets = a_mid_ifrow.InUcastPkts;
> > +            stats->rx_errs = a_mid_ifrow.InErrors;
> > +            stats->rx_dropped = a_mid_ifrow.InDiscards;
> > +            stats->tx_bytes = a_mid_ifrow.OutOctets;
> > +            stats->tx_packets = a_mid_ifrow.OutUcastPkts;
> > +            stats->tx_errs = a_mid_ifrow.OutErrors;
> > +            stats->tx_dropped = a_mid_ifrow.OutDiscards;
> >              return 0;
> >          }
> >      }
> > 
> > I have the modified patch staged here:
> > 
> >   https://github.com/mdroth/qemu/commit/
> > de597a8b27722ce4f9cc660f930f7dccc712712d
> > 
> > Let me know if it looks okay and I'll send a pull request for rc1
> > 
> > > +        }
> > >      }
> > >      return -1;
> > >  }
> > > -- 
> > > 1.8.3.1
> > > 
> > 
> > 
> 
>