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

Yuanhan Liu yuanhan.liu at linux.intel.com
Tue Sep 6 14:27:43 CEST 2016


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 at 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 at dpdk.org>

So, mind to send v3, with above fixes?

Thanks.

	--yliu


More information about the dev mailing list