[dpdk-dev,v2] net/virtio: fix xstats name issue

  The patch fixes some xstats name issues and make the xstats name conform
to the definition of etherStatsPkts1024to1518Octets in rfc2819 page 23.

Fixes: 76d4c652e07d ("virtio: add extended stats")

---

Changes in v2:

modify commit summary.
add the fixline.

Signed-off-by: Zhiyong Yang <zhiyong.yang@intel.com>
---
 drivers/net/virtio/virtio_ethdev.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
  

On Tue, Sep 06, 2016 at 03:50:12PM +0800, Zhiyong Yang wrote:
> The patch fixes some xstats name issues

Please, state **clearly** what the issue is: it's far away from being
enough just mentioning "fix a issue" without actually telling what it
is.

For this case, you could describe the issue like:

    We have a stats named "size_1024_1517_packets", while the code
    actually counts the range "[1024, 1518]", which is obviously wrong.

You could even add the related code piece here (you see the context
is missing in the patch, which is harder for reviewer to see what's
actually going wrong):

	else if (s < 1519)
		stats->size_bins[6]++;
    
> and make the xstats name conform
> to the definition of etherStatsPkts1024to1518Octets in rfc2819 page 23.

It's a bit abrupt to metion the RFC here, without some background. It
could be better if we mention something like: (just followed by above
issue description).

    We could either fix it by correcting the "if" check in the code,
    or fix it by just renaming the stats to conform to the code. The
    later solution is taken because that's what the RFC2819 suggests.


> Fixes: 76d4c652e07d ("virtio: add extended stats")
> 
> ---

Besides that, the three dash "---" means the end of your commit log,
therefore, it should go --->

> 
> Changes in v2:
> 
> modify commit summary.
> add the fixline.
> 
> Signed-off-by: Zhiyong Yang <zhiyong.yang@intel.com>

... here

Otherwise, your SoB will be chopped off while apply.

Another thing to note is, it's a good candidate to me for stable branch,
thus, you may need add following in the commit log before you SoB:

    Cc: <stable@dpdk.org>

So, mind to send v3, with above fixes?

Thanks.

	--yliu
  

hi, yuanhan:

Thanks a lot  for your clear comments in detail. 
Patch v3 will be sent later.

-zhiyong-

> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: Tuesday, September 6, 2016 8:28 PM
> To: Yang, Zhiyong <zhiyong.yang@intel.com>
> Cc: dev@dpdk.org; Xie, Huawei <huawei.xie@intel.com>
> Subject: Re: [PATCH v2] net/virtio: fix xstats name issue
> 
> On Tue, Sep 06, 2016 at 03:50:12PM +0800, Zhiyong Yang wrote:
> > The patch fixes some xstats name issues
> 
> Please, state **clearly** what the issue is: it's far away from being enough
> just mentioning "fix a issue" without actually telling what it is.
> 
> For this case, you could describe the issue like:
> 
>     We have a stats named "size_1024_1517_packets", while the code
>     actually counts the range "[1024, 1518]", which is obviously wrong.
> 
> You could even add the related code piece here (you see the context is
> missing in the patch, which is harder for reviewer to see what's actually going
> wrong):
> 
> 	else if (s < 1519)
> 		stats->size_bins[6]++;
> 
> > and make the xstats name conform
> > to the definition of etherStatsPkts1024to1518Octets in rfc2819 page 23.
> 
> It's a bit abrupt to metion the RFC here, without some background. It could
> be better if we mention something like: (just followed by above issue
> description).
> 
>     We could either fix it by correcting the "if" check in the code,
>     or fix it by just renaming the stats to conform to the code. The
>     later solution is taken because that's what the RFC2819 suggests.
> 
> 
> > Fixes: 76d4c652e07d ("virtio: add extended stats")
> >
> > ---
> 
> Besides that, the three dash "---" means the end of your commit log,
> therefore, it should go --->
> 
> >
> > Changes in v2:
> >
> > modify commit summary.
> > add the fixline.
> >
> > Signed-off-by: Zhiyong Yang <zhiyong.yang@intel.com>
> 
> ... here
> 
> Otherwise, your SoB will be chopped off while apply.
> 
> Another thing to note is, it's a good candidate to me for stable branch, thus,
> you may need add following in the commit log before you SoB:
> 
>     Cc: <stable@dpdk.org>
> 
> So, mind to send v3, with above fixes?
> 
> Thanks.
> 
> 	--yliu