| Message ID | 1418835808-18803-1-git-send-email-nhorman@tuxdriver.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 49DF38097; Wed, 17 Dec 2014 18:03:53 +0100 (CET) Received: from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58]) by dpdk.org (Postfix) with ESMTP id F0A20808A for <dev@dpdk.org>; Wed, 17 Dec 2014 18:03:49 +0100 (CET) Received: from hmsreliant.think-freely.org ([2001:470:8:a08:7aac:c0ff:fec2:933b] helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.63) (envelope-from <nhorman@tuxdriver.com>) id 1Y1I0k-00046Q-GQ; Wed, 17 Dec 2014 12:03:47 -0500 From: Neil Horman <nhorman@tuxdriver.com> To: dev@dpdk.org Date: Wed, 17 Dec 2014 12:03:28 -0500 Message-Id: <1418835808-18803-1-git-send-email-nhorman@tuxdriver.com> X-Mailer: git-send-email 1.9.3 X-Spam-Score: -2.9 (--) X-Spam-Status: No Subject: [dpdk-dev] [PATCH] xenvirt: Fix build break on cmdline_parse_etheraddr call X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Neil Horman
Dec. 17, 2014, 5:03 p.m. UTC
Back in:
commit aaa662e75c23c61a1d79bd4d1f9f35b4967c39db
Author: Alan Carew <alan.carew@intel.com>
Date: Fri Dec 5 15:19:07 2014 +0100
cmdline: fix overflow on bsd
The author failed to fixup a call to cmdline_parse_etheraddr in xenvirt. This
patch makes the needed correction to avoid a build break
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: Thomas Monjalon <thomas.monjalon@6wind.com>
---
lib/librte_pmd_xenvirt/rte_eth_xenvirt.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
Comments
Hi Neil, 2014-12-17 12:03, Neil Horman: > Back in: > > commit aaa662e75c23c61a1d79bd4d1f9f35b4967c39db > Author: Alan Carew <alan.carew@intel.com> > Date: Fri Dec 5 15:19:07 2014 +0100 > > cmdline: fix overflow on bsd > > The author failed to fixup a call to cmdline_parse_etheraddr in xenvirt. This > patch makes the needed correction to avoid a build break > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com> > CC: Thomas Monjalon <thomas.monjalon@6wind.com> What is the meaning of CC here? > --- a/lib/librte_pmd_xenvirt/rte_eth_xenvirt.c > +++ b/lib/librte_pmd_xenvirt/rte_eth_xenvirt.c > @@ -586,8 +586,9 @@ rte_eth_xenvirt_parse_args(struct xenvirt_dict *dict, > if (!strncmp(pair[0], RTE_ETH_XENVIRT_MAC_PARAM, > sizeof(RTE_ETH_XENVIRT_MAC_PARAM))) { > if (cmdline_parse_etheraddr(NULL, > - pair[1], > - &dict->addr) < 0) { > + pair[1], > + &dict->addr, > + sizeof(struct ether_addr)) < 0) { Why not sizeof(dict->addr)?
Hi Neil, Can you let me know what configuration you used for XEN Domain U? Do you able to run some test cases on XEN Domain U? So far, we met some issues on xenvirt (Domain U). Thanks Waterman
Hi Neil, On 12/17/2014 06:03 PM, Neil Horman wrote: > Back in: > > commit aaa662e75c23c61a1d79bd4d1f9f35b4967c39db > Author: Alan Carew <alan.carew@intel.com> > Date: Fri Dec 5 15:19:07 2014 +0100 > > cmdline: fix overflow on bsd > > The author failed to fixup a call to cmdline_parse_etheraddr in xenvirt. This > patch makes the needed correction to avoid a build break > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com> Sorry, I missed that one, thank you for fixing it. Acked-by: Olivier Matz <olivier.matz@6wind.com>
Hi, On 12/18/2014 09:40 AM, Olivier MATZ wrote: > Hi Neil, > > On 12/17/2014 06:03 PM, Neil Horman wrote: >> Back in: >> >> commit aaa662e75c23c61a1d79bd4d1f9f35b4967c39db >> Author: Alan Carew <alan.carew@intel.com> >> Date: Fri Dec 5 15:19:07 2014 +0100 >> >> cmdline: fix overflow on bsd >> >> The author failed to fixup a call to cmdline_parse_etheraddr in xenvirt. This >> patch makes the needed correction to avoid a build break >> >> Signed-off-by: Neil Horman <nhorman@tuxdriver.com> > > Sorry, I missed that one, thank you for fixing it. > > Acked-by: Olivier Matz <olivier.matz@6wind.com> Hmm, I agree with Thomas that sizeof(dict->addr) would be better as it explicitly uses the length of the field we write into. Regards, Olivier
On Wed, Dec 17, 2014 at 11:20:26PM +0100, Thomas Monjalon wrote: > Hi Neil, > > 2014-12-17 12:03, Neil Horman: > > Back in: > > > > commit aaa662e75c23c61a1d79bd4d1f9f35b4967c39db > > Author: Alan Carew <alan.carew@intel.com> > > Date: Fri Dec 5 15:19:07 2014 +0100 > > > > cmdline: fix overflow on bsd > > > > The author failed to fixup a call to cmdline_parse_etheraddr in xenvirt. This > > patch makes the needed correction to avoid a build break > > > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com> > > CC: Thomas Monjalon <thomas.monjalon@6wind.com> > > What is the meaning of CC here? > CC is a tag that git send-email understands. As it implies it cc's the post to the indicated email, and records that fact in the body of the commit. > > --- a/lib/librte_pmd_xenvirt/rte_eth_xenvirt.c > > +++ b/lib/librte_pmd_xenvirt/rte_eth_xenvirt.c > > @@ -586,8 +586,9 @@ rte_eth_xenvirt_parse_args(struct xenvirt_dict *dict, > > if (!strncmp(pair[0], RTE_ETH_XENVIRT_MAC_PARAM, > > sizeof(RTE_ETH_XENVIRT_MAC_PARAM))) { > > if (cmdline_parse_etheraddr(NULL, > > - pair[1], > > - &dict->addr) < 0) { > > + pair[1], > > + &dict->addr, > > + sizeof(struct ether_addr)) < 0) { > > Why not sizeof(dict->addr)? > Because addr is a struct ether_addr, and I always get confused when doing sizeof on pointer variables, so I find it more clear to specify the type exactly. I'm not bound to it though so if you like I can change it, though given its release day, I figure you want to fix this build break asap. Neil > -- > Thomas >
On Thu, Dec 18, 2014 at 03:40:54AM +0000, Cao, Waterman wrote: > Hi Neil, > > Can you let me know what configuration you used for XEN Domain U? > Do you able to run some test cases on XEN Domain U? > So far, we met some issues on xenvirt (Domain U). > > Thanks > > Waterman > No configuration, I simply turned on the option in the build to ensure another patch set of mine didn't break anything. Neil
2014-12-18 06:25, Neil Horman: > On Wed, Dec 17, 2014 at 11:20:26PM +0100, Thomas Monjalon wrote: > > 2014-12-17 12:03, Neil Horman: > > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com> > > > CC: Thomas Monjalon <thomas.monjalon@6wind.com> > > > > What is the meaning of CC here? > > > CC is a tag that git send-email understands. As it implies it cc's the post to > the indicated email, and records that fact in the body of the commit. OK but why CC me when sending this patch?
On 2014/12/18 19:26, Neil Horman wrote: > On Wed, Dec 17, 2014 at 11:20:26PM +0100, Thomas Monjalon wrote: >> Hi Neil, >> >> 2014-12-17 12:03, Neil Horman: >>> Back in: >>> >>> commit aaa662e75c23c61a1d79bd4d1f9f35b4967c39db >>> Author: Alan Carew <alan.carew@intel.com> >>> Date: Fri Dec 5 15:19:07 2014 +0100 >>> >>> cmdline: fix overflow on bsd >>> >>> The author failed to fixup a call to cmdline_parse_etheraddr in xenvirt. This >>> patch makes the needed correction to avoid a build break >>> >>> Signed-off-by: Neil Horman <nhorman@tuxdriver.com> >>> CC: Thomas Monjalon <thomas.monjalon@6wind.com> >> What is the meaning of CC here? >> > CC is a tag that git send-email understands. As it implies it cc's the post to > the indicated email, and records that fact in the body of the commit. But if you use --cc in git send-email will be a good choice. CC list is useless for patch it self, but here it will exist in commit log(although this style can also be seen in linux kernel) Thanks, Michael >>> --- a/lib/librte_pmd_xenvirt/rte_eth_xenvirt.c >>> +++ b/lib/librte_pmd_xenvirt/rte_eth_xenvirt.c >>> @@ -586,8 +586,9 @@ rte_eth_xenvirt_parse_args(struct xenvirt_dict *dict, >>> if (!strncmp(pair[0], RTE_ETH_XENVIRT_MAC_PARAM, >>> sizeof(RTE_ETH_XENVIRT_MAC_PARAM))) { >>> if (cmdline_parse_etheraddr(NULL, >>> - pair[1], >>> - &dict->addr) < 0) { >>> + pair[1], >>> + &dict->addr, >>> + sizeof(struct ether_addr)) < 0) { >> Why not sizeof(dict->addr)? >> > Because addr is a struct ether_addr, and I always get confused when doing sizeof > on pointer variables, so I find it more clear to specify the type exactly. I'm > not bound to it though so if you like I can change it, though given its release > day, I figure you want to fix this build break asap. > Neil > >> -- >> Thomas >>
On Wed, 17 Dec 2014 12:03:28 -0500 Neil Horman <nhorman@tuxdriver.com> wrote: > Back in: > > commit aaa662e75c23c61a1d79bd4d1f9f35b4967c39db > Author: Alan Carew <alan.carew@intel.com> > Date: Fri Dec 5 15:19:07 2014 +0100 > > cmdline: fix overflow on bsd > > The author failed to fixup a call to cmdline_parse_etheraddr in xenvirt. This > patch makes the needed correction to avoid a build break > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com> > CC: Thomas Monjalon <thomas.monjalon@6wind.com> If we could fix the header incompatiablities then the driver could use a standard library like ether_aton() instead of dragging in the unnecessary cmdline library.
2014-12-18 08:17, Stephen Hemminger: > On Wed, 17 Dec 2014 12:03:28 -0500 > Neil Horman <nhorman@tuxdriver.com> wrote: [...] > > The author failed to fixup a call to cmdline_parse_etheraddr in xenvirt. This > > patch makes the needed correction to avoid a build break [...] > > If we could fix the header incompatiablities then the driver could use > a standard library like ether_aton() instead of dragging in the unnecessary > cmdline library. Right. A first fix was done to remove conflict with netinet/in.h: http://dpdk.org/browse/dpdk/commit/?id=d07180f211 But there still are conflicts with netinet/ether.h and maybe more. I suggest to fix it in the release 2.0.
On Thu, Dec 18, 2014 at 01:21:31PM +0100, Thomas Monjalon wrote: > 2014-12-18 06:25, Neil Horman: > > On Wed, Dec 17, 2014 at 11:20:26PM +0100, Thomas Monjalon wrote: > > > 2014-12-17 12:03, Neil Horman: > > > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com> > > > > CC: Thomas Monjalon <thomas.monjalon@6wind.com> > > > > > > What is the meaning of CC here? > > > > > CC is a tag that git send-email understands. As it implies it cc's the post to > > the indicated email, and records that fact in the body of the commit. > > OK but why CC me when sending this patch? Typically a tree maintainer is CC'ed when submitting a patch to a list. I've done it on most, if not all of my previous patches (and where I didn't, I should have). Especially given that we're this late in the release cycle, I want to be sure you're attention is called to a build break Neil > > -- > Thomas >
On Thu, Dec 18, 2014 at 01:51:13PM +0000, Qiu, Michael wrote: > On 2014/12/18 19:26, Neil Horman wrote: > > On Wed, Dec 17, 2014 at 11:20:26PM +0100, Thomas Monjalon wrote: > >> Hi Neil, > >> > >> 2014-12-17 12:03, Neil Horman: > >>> Back in: > >>> > >>> commit aaa662e75c23c61a1d79bd4d1f9f35b4967c39db > >>> Author: Alan Carew <alan.carew@intel.com> > >>> Date: Fri Dec 5 15:19:07 2014 +0100 > >>> > >>> cmdline: fix overflow on bsd > >>> > >>> The author failed to fixup a call to cmdline_parse_etheraddr in xenvirt. This > >>> patch makes the needed correction to avoid a build break > >>> > >>> Signed-off-by: Neil Horman <nhorman@tuxdriver.com> > >>> CC: Thomas Monjalon <thomas.monjalon@6wind.com> > >> What is the meaning of CC here? > >> > > CC is a tag that git send-email understands. As it implies it cc's the post to > > the indicated email, and records that fact in the body of the commit. > > But if you use --cc in git send-email will be a good choice. CC list is > useless for patch it self, but here it will exist in commit log(although > this style can also be seen in linux kernel) Yes, its good to have a record of who was CCed on a patch for archival purposes, so you can get an idea of who participated (or was expected to participate in a review) Neil > > Thanks, > Michael > >>> --- a/lib/librte_pmd_xenvirt/rte_eth_xenvirt.c > >>> +++ b/lib/librte_pmd_xenvirt/rte_eth_xenvirt.c > >>> @@ -586,8 +586,9 @@ rte_eth_xenvirt_parse_args(struct xenvirt_dict *dict, > >>> if (!strncmp(pair[0], RTE_ETH_XENVIRT_MAC_PARAM, > >>> sizeof(RTE_ETH_XENVIRT_MAC_PARAM))) { > >>> if (cmdline_parse_etheraddr(NULL, > >>> - pair[1], > >>> - &dict->addr) < 0) { > >>> + pair[1], > >>> + &dict->addr, > >>> + sizeof(struct ether_addr)) < 0) { > >> Why not sizeof(dict->addr)? > >> > > Because addr is a struct ether_addr, and I always get confused when doing sizeof > > on pointer variables, so I find it more clear to specify the type exactly. I'm > > not bound to it though so if you like I can change it, though given its release > > day, I figure you want to fix this build break asap. > > Neil > > > >> -- > >> Thomas > >> > >
On Thu, Dec 18, 2014 at 08:17:12AM -0800, Stephen Hemminger wrote: > On Wed, 17 Dec 2014 12:03:28 -0500 > Neil Horman <nhorman@tuxdriver.com> wrote: > > > Back in: > > > > commit aaa662e75c23c61a1d79bd4d1f9f35b4967c39db > > Author: Alan Carew <alan.carew@intel.com> > > Date: Fri Dec 5 15:19:07 2014 +0100 > > > > cmdline: fix overflow on bsd > > > > The author failed to fixup a call to cmdline_parse_etheraddr in xenvirt. This > > patch makes the needed correction to avoid a build break > > > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com> > > CC: Thomas Monjalon <thomas.monjalon@6wind.com> > > If we could fix the header incompatiablities then the driver could use > a standard library like ether_aton() instead of dragging in the unnecessary > cmdline library. > I agree, that would be great, but I think that would be better done after the release, since this is in compliance with how the rest of the DPDK handles this situation currently. Using ether_aton is definately a superior solution however. Neil
diff --git a/lib/librte_pmd_xenvirt/rte_eth_xenvirt.c b/lib/librte_pmd_xenvirt/rte_eth_xenvirt.c index 6555ec5..6fa6758 100644 --- a/lib/librte_pmd_xenvirt/rte_eth_xenvirt.c +++ b/lib/librte_pmd_xenvirt/rte_eth_xenvirt.c @@ -586,8 +586,9 @@ rte_eth_xenvirt_parse_args(struct xenvirt_dict *dict, if (!strncmp(pair[0], RTE_ETH_XENVIRT_MAC_PARAM, sizeof(RTE_ETH_XENVIRT_MAC_PARAM))) { if (cmdline_parse_etheraddr(NULL, - pair[1], - &dict->addr) < 0) { + pair[1], + &dict->addr, + sizeof(struct ether_addr)) < 0) { RTE_LOG(ERR, PMD, "Invalid %s device ether address\n", name);