[dpdk-dev] xenvirt: Fix build break on cmdline_parse_etheraddr call
diff mbox

Message ID 1418835808-18803-1-git-send-email-nhorman@tuxdriver.com
State Superseded, archived
Headers show

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

Thomas Monjalon Dec. 17, 2014, 10:20 p.m. UTC | #1
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)?
Cao, Waterman Dec. 18, 2014, 3:40 a.m. UTC | #2
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
Olivier Matz Dec. 18, 2014, 8:40 a.m. UTC | #3
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>
Olivier Matz Dec. 18, 2014, 9:26 a.m. UTC | #4
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
Neil Horman Dec. 18, 2014, 11:25 a.m. UTC | #5
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
>
Neil Horman Dec. 18, 2014, 11:26 a.m. UTC | #6
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
Thomas Monjalon Dec. 18, 2014, 12:21 p.m. UTC | #7
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?
Michael Qiu Dec. 18, 2014, 1:51 p.m. UTC | #8
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
>>
Stephen Hemminger Dec. 18, 2014, 4:17 p.m. UTC | #9
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.
Thomas Monjalon Dec. 18, 2014, 4:40 p.m. UTC | #10
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.
Neil Horman Dec. 18, 2014, 7:57 p.m. UTC | #11
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
>
Neil Horman Dec. 18, 2014, 7:58 p.m. UTC | #12
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
> >>
> 
>
Neil Horman Dec. 18, 2014, 8:57 p.m. UTC | #13
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

Patch
diff mbox

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);