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 151D258D8 for ; Wed, 7 Sep 2016 03:32:33 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga102.jf.intel.com with ESMTP; 06 Sep 2016 18:32:32 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.30,294,1470726000"; d="scan'208";a="1036339668" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga001.fm.intel.com with ESMTP; 06 Sep 2016 18:32:32 -0700 Received: from fmsmsx154.amr.corp.intel.com (10.18.116.70) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 6 Sep 2016 18:32:32 -0700 Received: from bgsmsx104.gar.corp.intel.com (10.223.4.190) by FMSMSX154.amr.corp.intel.com (10.18.116.70) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 6 Sep 2016 18:32:32 -0700 Received: from bgsmsx101.gar.corp.intel.com ([169.254.1.182]) by BGSMSX104.gar.corp.intel.com ([169.254.5.113]) with mapi id 14.03.0248.002; Wed, 7 Sep 2016 07:02:29 +0530 From: "Yang, Zhiyong" To: Yuanhan Liu CC: "dev@dpdk.org" , "Xie, Huawei" Thread-Topic: [PATCH v2] net/virtio: fix xstats name issue Thread-Index: AQHSCBOkYIc6H18S4UufUoEHoXycIKBsB8aAgAEzb6A= Date: Wed, 7 Sep 2016 01:32:28 +0000 Message-ID: References: <1472716874-34036-1-git-send-email-zhiyong.yang@intel.com> <1473148212-142486-1-git-send-email-zhiyong.yang@intel.com> <20160906122743.GD23158@yliu-dev.sh.intel.com> In-Reply-To: <20160906122743.GD23158@yliu-dev.sh.intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNTQ3ZmE1NjQtN2VhYi00MzgyLWFhMTQtOTIwZGI0ZjhmZWM3IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6IkNFSXlERlVKVEdMaFRudmpjUjZkZjVrUnI4a3NSUFN6MmVBSWhKanNGWVk9In0= x-ctpclassification: CTP_IC x-originating-ip: [10.223.10.10] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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: Wed, 07 Sep 2016 01:32:34 -0000 hi, yuanhan: Thanks a lot for your clear comments in detail.=20 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 > Cc: dev@dpdk.org; Xie, Huawei > Subject: Re: [PATCH v2] net/virtio: fix xstats name issue >=20 > On Tue, Sep 06, 2016 at 03:50:12PM +0800, Zhiyong Yang wrote: > > The patch fixes some xstats name issues >=20 > Please, state **clearly** what the issue is: it's far away from being eno= ugh > just mentioning "fix a issue" without actually telling what it is. >=20 > For this case, you could describe the issue like: >=20 > We have a stats named "size_1024_1517_packets", while the code > actually counts the range "[1024, 1518]", which is obviously wrong. >=20 > 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): >=20 > else if (s < 1519) > stats->size_bins[6]++; >=20 > > and make the xstats name conform > > to the definition of etherStatsPkts1024to1518Octets in rfc2819 page 23. >=20 > It's a bit abrupt to metion the RFC here, without some background. It cou= ld > be better if we mention something like: (just followed by above issue > description). >=20 > 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. >=20 >=20 > > Fixes: 76d4c652e07d ("virtio: add extended stats") > > > > --- >=20 > Besides that, the three dash "---" means the end of your commit log, > therefore, it should go ---> >=20 > > > > Changes in v2: > > > > modify commit summary. > > add the fixline. > > > > Signed-off-by: Zhiyong Yang >=20 > ... here >=20 > Otherwise, your SoB will be chopped off while apply. >=20 > 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: >=20 > Cc: >=20 > So, mind to send v3, with above fixes? >=20 > Thanks. >=20 > --yliu