Re: [Qemu-devel] [PATCH v2] qga: Add support network interface statistics in guest-network-get-interfaces command

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3429 bytes --]

On 04/20/2017 05:42 AM, ZhiPeng Lu wrote:
> we can get the  network inetrface statistics inside  a virtual machine by

s/inetrface/interface/

lose the double spaces (twice)

> guest-network-get-interfaces command.
> it is very userful for us to monitor and analyze network traff.

s/userful/useful/
s/traff/traffic/

> 
> Signed-off-by: ZhiPeng Lu <lu.zhipeng@zte.com.cn>
> Signed-off-by: DanielP.Berrange <berrange@redhat.com>
> Reviewed-by: EricBlake  <eblake@redhat.com>

Please fix the spacing of Daniel and my names to match the spacing we
typically use (I like to visit 'git log' to learn statistics of patches
I've helped on, and incorrect spellings are easy to miss).

Worse, you added Reviewed-by: attributed to me when I did NOT authorize
it on v1:
https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg03026.html

I pointed out that your v1 was incomplete.  Sometimes, I authorize a
conditional R-b, where I state what trivial action must be taken to fix
the patch without needing me to re-read things - but in the case of
missing documentation, where there is a good chance that it may need
wording improvements, I prefer to omit my R-b and read v2 in its entirety.

> +++ b/qga/qapi-schema.json
> @@ -635,6 +635,31 @@
>             'prefix': 'int'} }
>  
>  ##
> +# @GuestNetworkInterfaceStat:
> +#
> +# @rx-bytes: received bytes of interface
> +# @rx-packets: received packets of interface
> +# @rx-errs: received error packets of interface
> +# @rx-drop: received drop packets of interface

s/ of interface//g - it's not adding any useful information.

How can you receive a dropped packet?  Obviously, the statistic is
available, but what is it really representing?  Maybe 'dropped packets
detected on receive side' reads better.

> +#
> +# @tx-bytes: send bytes of interface
> +# @tx-packets: send packets of interface

s/send/transmitted/

> +# @tx-errs: send error packets of interface

It's not how many error packets were sent, but how many errors were
detected while attempting to send, so maybe 'error packets detected on
transmission side'

> +# @tx-drop: send drop packets of interface
and 'dropped packets detected on transmission side'

> +# Since: 2.10
> +##
> +{ 'struct': 'GuestNetworkInterfaceStat',
> +  'data': {'rx-bytes': 'uint64',
> +            'rx-packets': 'uint64',
> +            'rx-errs': 'uint64',
> +            'rx-drop': 'uint64',
> +            'tx-bytes': 'uint64',
> +            'tx-packets': 'uint64',
> +            'tx-errs': 'uint64',
> +            'tx-drop': 'uint64'
> +           } }
> +
> +##
>  # @GuestNetworkInterface:
>  #
>  # @name: The name of interface for which info are being delivered
> @@ -648,7 +673,8 @@
>  { 'struct': 'GuestNetworkInterface',
>    'data': {'name': 'str',
>             '*hardware-address': 'str',
> -           '*ip-addresses': ['GuestIpAddress'] } }
> +           '*ip-addresses': ['GuestIpAddress'],
> +           '*interface-statics': ['GuestNetworkInterfaceStat'] } }

s/interface-statics/statistics/ (the 'interface-' prefix is redundant,
and you typo'd the remaining portion of the name)

Missing documentation, including a (since 2.10) tag.

So you still do not have my R-b; looking forward to v3.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]