From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 8957E5A0C for ; Tue, 6 Sep 2016 14:15:35 +0200 (CEST) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga102.jf.intel.com with ESMTP; 06 Sep 2016 05:15:35 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.30,291,1470726000"; d="scan'208";a="1046260998" Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.67.162]) by orsmga002.jf.intel.com with ESMTP; 06 Sep 2016 05:15:33 -0700 Date: Tue, 6 Sep 2016 20:27:43 +0800 From: Yuanhan Liu To: Zhiyong Yang Cc: dev@dpdk.org, huawei.xie@intel.com Message-ID: <20160906122743.GD23158@yliu-dev.sh.intel.com> References: <1472716874-34036-1-git-send-email-zhiyong.yang@intel.com> <1473148212-142486-1-git-send-email-zhiyong.yang@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1473148212-142486-1-git-send-email-zhiyong.yang@intel.com> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH v2] net/virtio: fix xstats name issue X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 06 Sep 2016 12:15:35 -0000 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 ... 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: So, mind to send v3, with above fixes? Thanks. --yliu