* [dpdk-dev] [PATCH] librte_eal: fix wrong args operation in eal_parse_args @ 2016-05-11 5:28 Ziye Yang 2016-05-11 11:24 ` Bruce Richardson 0 siblings, 1 reply; 6+ messages in thread From: Ziye Yang @ 2016-05-11 5:28 UTC (permalink / raw) To: dev This patch is used to fix wrong operation on user input args. eal_parse_args function should not operate the args passed by the user. If the element in argv is generated by malloc function, changing it will cause memory issues when free the args. Signed-off-by: Ziye Yang <ziye.yang@intel.com> --- lib/librte_eal/bsdapp/eal/eal.c | 2 -- lib/librte_eal/linuxapp/eal/eal.c | 2 -- 2 files changed, 4 deletions(-) diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c index 06bfd4e..0eef92d 100644 --- a/lib/librte_eal/bsdapp/eal/eal.c +++ b/lib/librte_eal/bsdapp/eal/eal.c @@ -420,8 +420,6 @@ eal_parse_args(int argc, char **argv) goto out; } - if (optind >= 0) - argv[optind-1] = prgname; ret = optind-1; out: diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c index 8aafd51..ba9d1ac 100644 --- a/lib/librte_eal/linuxapp/eal/eal.c +++ b/lib/librte_eal/linuxapp/eal/eal.c @@ -658,8 +658,6 @@ eal_parse_args(int argc, char **argv) goto out; } - if (optind >= 0) - argv[optind-1] = prgname; ret = optind-1; out: -- 1.9.3 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH] librte_eal: fix wrong args operation in eal_parse_args 2016-05-11 5:28 [dpdk-dev] [PATCH] librte_eal: fix wrong args operation in eal_parse_args Ziye Yang @ 2016-05-11 11:24 ` Bruce Richardson 2016-05-11 11:51 ` Yang, Ziye 0 siblings, 1 reply; 6+ messages in thread From: Bruce Richardson @ 2016-05-11 11:24 UTC (permalink / raw) To: Ziye Yang; +Cc: dev On Wed, May 11, 2016 at 01:28:21PM +0800, Ziye Yang wrote: > This patch is used to fix wrong operation on user > input args. eal_parse_args function should not operate > the args passed by the user. If the element in argv > is generated by malloc function, changing it will cause > memory issues when free the args. > > Signed-off-by: Ziye Yang <ziye.yang@intel.com> > --- > lib/librte_eal/bsdapp/eal/eal.c | 2 -- > lib/librte_eal/linuxapp/eal/eal.c | 2 -- > 2 files changed, 4 deletions(-) > > diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c > index 06bfd4e..0eef92d 100644 > --- a/lib/librte_eal/bsdapp/eal/eal.c > +++ b/lib/librte_eal/bsdapp/eal/eal.c > @@ -420,8 +420,6 @@ eal_parse_args(int argc, char **argv) > goto out; > } > > - if (optind >= 0) > - argv[optind-1] = prgname; > ret = optind-1; > > out: > diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c > index 8aafd51..ba9d1ac 100644 > --- a/lib/librte_eal/linuxapp/eal/eal.c > +++ b/lib/librte_eal/linuxapp/eal/eal.c > @@ -658,8 +658,6 @@ eal_parse_args(int argc, char **argv) > goto out; > } > > - if (optind >= 0) > - argv[optind-1] = prgname; > ret = optind-1; > > out: This is a behaviour change in DPDK. The behaviour has always been that after calling eal init, you can update your argv/argc values by the number of args parsed and then parse your app args as normal, since argv[0] will still point to your program name. While I agree that having the current behaviour may cause some problems, changing this behaviour may break applications that have been written to use the existing behaviour. Since it is only the last EAL parameter arg that is modified, I think it would be acceptable to have the behaviour well documented and then expect any app to store a second copy of the pointer to be modified if it is needed for a subsequent free call, for example. /Bruce ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH] librte_eal: fix wrong args operation in eal_parse_args 2016-05-11 11:24 ` Bruce Richardson @ 2016-05-11 11:51 ` Yang, Ziye 2016-05-11 12:21 ` Richardson, Bruce 0 siblings, 1 reply; 6+ messages in thread From: Yang, Ziye @ 2016-05-11 11:51 UTC (permalink / raw) To: Richardson, Bruce; +Cc: dev Hi Bruce, Could it be fixed later? If not, it should be documented. I faced this issued today, and found that dpdk changed my last arg. In my mind, dpdk should not change the argv[last], which will confuse the users. Best Regards, Ziye Yang -----Original Message----- From: Richardson, Bruce Sent: Wednesday, May 11, 2016 7:25 PM To: Yang, Ziye <ziye.yang@intel.com> Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH] librte_eal: fix wrong args operation in eal_parse_args On Wed, May 11, 2016 at 01:28:21PM +0800, Ziye Yang wrote: > This patch is used to fix wrong operation on user input args. > eal_parse_args function should not operate the args passed by the > user. If the element in argv is generated by malloc function, changing > it will cause memory issues when free the args. > > Signed-off-by: Ziye Yang <ziye.yang@intel.com> > --- > lib/librte_eal/bsdapp/eal/eal.c | 2 -- > lib/librte_eal/linuxapp/eal/eal.c | 2 -- > 2 files changed, 4 deletions(-) > > diff --git a/lib/librte_eal/bsdapp/eal/eal.c > b/lib/librte_eal/bsdapp/eal/eal.c index 06bfd4e..0eef92d 100644 > --- a/lib/librte_eal/bsdapp/eal/eal.c > +++ b/lib/librte_eal/bsdapp/eal/eal.c > @@ -420,8 +420,6 @@ eal_parse_args(int argc, char **argv) > goto out; > } > > - if (optind >= 0) > - argv[optind-1] = prgname; > ret = optind-1; > > out: > diff --git a/lib/librte_eal/linuxapp/eal/eal.c > b/lib/librte_eal/linuxapp/eal/eal.c > index 8aafd51..ba9d1ac 100644 > --- a/lib/librte_eal/linuxapp/eal/eal.c > +++ b/lib/librte_eal/linuxapp/eal/eal.c > @@ -658,8 +658,6 @@ eal_parse_args(int argc, char **argv) > goto out; > } > > - if (optind >= 0) > - argv[optind-1] = prgname; > ret = optind-1; > > out: This is a behaviour change in DPDK. The behaviour has always been that after calling eal init, you can update your argv/argc values by the number of args parsed and then parse your app args as normal, since argv[0] will still point to your program name. While I agree that having the current behaviour may cause some problems, changing this behaviour may break applications that have been written to use the existing behaviour. Since it is only the last EAL parameter arg that is modified, I think it would be acceptable to have the behaviour well documented and then expect any app to store a second copy of the pointer to be modified if it is needed for a subsequent free call, for example. /Bruce ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH] librte_eal: fix wrong args operation in eal_parse_args 2016-05-11 11:51 ` Yang, Ziye @ 2016-05-11 12:21 ` Richardson, Bruce 2016-05-11 13:39 ` Yang, Ziye 0 siblings, 1 reply; 6+ messages in thread From: Richardson, Bruce @ 2016-05-11 12:21 UTC (permalink / raw) To: Yang, Ziye; +Cc: dev > -----Original Message----- > From: Yang, Ziye > Sent: Wednesday, May 11, 2016 12:51 PM > To: Richardson, Bruce <bruce.richardson@intel.com> > Cc: dev@dpdk.org > Subject: RE: [dpdk-dev] [PATCH] librte_eal: fix wrong args operation in > eal_parse_args > > Hi Bruce, > > Could it be fixed later? If not, it should be documented. I faced this > issued today, and found that dpdk changed my last arg. In my mind, dpdk > should not change the argv[last], which will confuse the users. > We can certainly consider changing it, but I am concerned about stability of existing apps. I think it needs some discussion and consensus on-list before a change like this is made. For right now, definitely the behavior should be documented. Would you consider submitting a documentation update patch for this issue instead of this code change? Thanks, /Bruce > Best Regards, > Ziye Yang > > -----Original Message----- > From: Richardson, Bruce > Sent: Wednesday, May 11, 2016 7:25 PM > To: Yang, Ziye <ziye.yang@intel.com> > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH] librte_eal: fix wrong args operation in > eal_parse_args > > On Wed, May 11, 2016 at 01:28:21PM +0800, Ziye Yang wrote: > > This patch is used to fix wrong operation on user input args. > > eal_parse_args function should not operate the args passed by the > > user. If the element in argv is generated by malloc function, changing > > it will cause memory issues when free the args. > > > > Signed-off-by: Ziye Yang <ziye.yang@intel.com> > > --- > > lib/librte_eal/bsdapp/eal/eal.c | 2 -- > > lib/librte_eal/linuxapp/eal/eal.c | 2 -- > > 2 files changed, 4 deletions(-) > > > > diff --git a/lib/librte_eal/bsdapp/eal/eal.c > > b/lib/librte_eal/bsdapp/eal/eal.c index 06bfd4e..0eef92d 100644 > > --- a/lib/librte_eal/bsdapp/eal/eal.c > > +++ b/lib/librte_eal/bsdapp/eal/eal.c > > @@ -420,8 +420,6 @@ eal_parse_args(int argc, char **argv) > > goto out; > > } > > > > - if (optind >= 0) > > - argv[optind-1] = prgname; > > ret = optind-1; > > > > out: > > diff --git a/lib/librte_eal/linuxapp/eal/eal.c > > b/lib/librte_eal/linuxapp/eal/eal.c > > index 8aafd51..ba9d1ac 100644 > > --- a/lib/librte_eal/linuxapp/eal/eal.c > > +++ b/lib/librte_eal/linuxapp/eal/eal.c > > @@ -658,8 +658,6 @@ eal_parse_args(int argc, char **argv) > > goto out; > > } > > > > - if (optind >= 0) > > - argv[optind-1] = prgname; > > ret = optind-1; > > > > out: > This is a behaviour change in DPDK. The behaviour has always been that > after calling eal init, you can update your argv/argc values by the number > of args parsed and then parse your app args as normal, since argv[0] will > still point to your program name. While I agree that having the current > behaviour may cause some problems, changing this behaviour may break > applications that have been written to use the existing behaviour. > > Since it is only the last EAL parameter arg that is modified, I think it > would be acceptable to have the behaviour well documented and then expect > any app to store a second copy of the pointer to be modified if it is > needed for a subsequent free call, for example. > > /Bruce ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH] librte_eal: fix wrong args operation in eal_parse_args 2016-05-11 12:21 ` Richardson, Bruce @ 2016-05-11 13:39 ` Yang, Ziye 2016-05-11 14:07 ` Richardson, Bruce 0 siblings, 1 reply; 6+ messages in thread From: Yang, Ziye @ 2016-05-11 13:39 UTC (permalink / raw) To: Richardson, Bruce; +Cc: dev Hi Bruce, Could you tell me the documentation file name? Then I can conduct the following documentation relate d patch. -----Original Message----- From: Richardson, Bruce Sent: Wednesday, May 11, 2016 8:21 PM To: Yang, Ziye <ziye.yang@intel.com> Cc: dev@dpdk.org Subject: RE: [dpdk-dev] [PATCH] librte_eal: fix wrong args operation in eal_parse_args > -----Original Message----- > From: Yang, Ziye > Sent: Wednesday, May 11, 2016 12:51 PM > To: Richardson, Bruce <bruce.richardson@intel.com> > Cc: dev@dpdk.org > Subject: RE: [dpdk-dev] [PATCH] librte_eal: fix wrong args operation > in eal_parse_args > > Hi Bruce, > > Could it be fixed later? If not, it should be documented. I faced > this issued today, and found that dpdk changed my last arg. In my > mind, dpdk should not change the argv[last], which will confuse the users. > We can certainly consider changing it, but I am concerned about stability of existing apps. I think it needs some discussion and consensus on-list before a change like this is made. For right now, definitely the behavior should be documented. Would you consider submitting a documentation update patch for this issue instead of this code change? Thanks, /Bruce > Best Regards, > Ziye Yang > > -----Original Message----- > From: Richardson, Bruce > Sent: Wednesday, May 11, 2016 7:25 PM > To: Yang, Ziye <ziye.yang@intel.com> > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH] librte_eal: fix wrong args operation > in eal_parse_args > > On Wed, May 11, 2016 at 01:28:21PM +0800, Ziye Yang wrote: > > This patch is used to fix wrong operation on user input args. > > eal_parse_args function should not operate the args passed by the > > user. If the element in argv is generated by malloc function, > > changing it will cause memory issues when free the args. > > > > Signed-off-by: Ziye Yang <ziye.yang@intel.com> > > --- > > lib/librte_eal/bsdapp/eal/eal.c | 2 -- > > lib/librte_eal/linuxapp/eal/eal.c | 2 -- > > 2 files changed, 4 deletions(-) > > > > diff --git a/lib/librte_eal/bsdapp/eal/eal.c > > b/lib/librte_eal/bsdapp/eal/eal.c index 06bfd4e..0eef92d 100644 > > --- a/lib/librte_eal/bsdapp/eal/eal.c > > +++ b/lib/librte_eal/bsdapp/eal/eal.c > > @@ -420,8 +420,6 @@ eal_parse_args(int argc, char **argv) > > goto out; > > } > > > > - if (optind >= 0) > > - argv[optind-1] = prgname; > > ret = optind-1; > > > > out: > > diff --git a/lib/librte_eal/linuxapp/eal/eal.c > > b/lib/librte_eal/linuxapp/eal/eal.c > > index 8aafd51..ba9d1ac 100644 > > --- a/lib/librte_eal/linuxapp/eal/eal.c > > +++ b/lib/librte_eal/linuxapp/eal/eal.c > > @@ -658,8 +658,6 @@ eal_parse_args(int argc, char **argv) > > goto out; > > } > > > > - if (optind >= 0) > > - argv[optind-1] = prgname; > > ret = optind-1; > > > > out: > This is a behaviour change in DPDK. The behaviour has always been that > after calling eal init, you can update your argv/argc values by the > number of args parsed and then parse your app args as normal, since > argv[0] will still point to your program name. While I agree that > having the current behaviour may cause some problems, changing this > behaviour may break applications that have been written to use the existing behaviour. > > Since it is only the last EAL parameter arg that is modified, I think > it would be acceptable to have the behaviour well documented and then > expect any app to store a second copy of the pointer to be modified if > it is needed for a subsequent free call, for example. > > /Bruce ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH] librte_eal: fix wrong args operation in eal_parse_args 2016-05-11 13:39 ` Yang, Ziye @ 2016-05-11 14:07 ` Richardson, Bruce 0 siblings, 0 replies; 6+ messages in thread From: Richardson, Bruce @ 2016-05-11 14:07 UTC (permalink / raw) To: Yang, Ziye; +Cc: dev > -----Original Message----- > From: Yang, Ziye > Sent: Wednesday, May 11, 2016 2:39 PM > To: Richardson, Bruce <bruce.richardson@intel.com> > Cc: dev@dpdk.org > Subject: RE: [dpdk-dev] [PATCH] librte_eal: fix wrong args operation in > eal_parse_args > > Hi Bruce, > > Could you tell me the documentation file name? Then I can conduct the > following documentation relate d patch. The behavior is already documented in the doxygen API definitions for rte_eal_init(). * @return$ * - On success, the number of parsed arguments, which is greater or$ * equal to zero. After the call to rte_eal_init(),$ * all arguments argv[x] with x < ret may be modified and should$ * not be accessed by the application.$ * - On failure, a negative error value.$ However, maybe you want to call it out in a different way to make it clearer for those using the functions. Other than that, perhaps look in the programmers guide document to see if it needs a mention there - or a reference to the API doc. /Bruce PS: When replying, please respond below existing emails, rather than top-posting. Thanks. > > -----Original Message----- > From: Richardson, Bruce > Sent: Wednesday, May 11, 2016 8:21 PM > To: Yang, Ziye <ziye.yang@intel.com> > Cc: dev@dpdk.org > Subject: RE: [dpdk-dev] [PATCH] librte_eal: fix wrong args operation in > eal_parse_args > > > > > -----Original Message----- > > From: Yang, Ziye > > Sent: Wednesday, May 11, 2016 12:51 PM > > To: Richardson, Bruce <bruce.richardson@intel.com> > > Cc: dev@dpdk.org > > Subject: RE: [dpdk-dev] [PATCH] librte_eal: fix wrong args operation > > in eal_parse_args > > > > Hi Bruce, > > > > Could it be fixed later? If not, it should be documented. I faced > > this issued today, and found that dpdk changed my last arg. In my > > mind, dpdk should not change the argv[last], which will confuse the > users. > > > > We can certainly consider changing it, but I am concerned about stability > of existing apps. I think it needs some discussion and consensus on-list > before a change like this is made. > > For right now, definitely the behavior should be documented. Would you > consider submitting a documentation update patch for this issue instead of > this code change? > > Thanks, > /Bruce > > > Best Regards, > > Ziye Yang > > > > -----Original Message----- > > From: Richardson, Bruce > > Sent: Wednesday, May 11, 2016 7:25 PM > > To: Yang, Ziye <ziye.yang@intel.com> > > Cc: dev@dpdk.org > > Subject: Re: [dpdk-dev] [PATCH] librte_eal: fix wrong args operation > > in eal_parse_args > > > > On Wed, May 11, 2016 at 01:28:21PM +0800, Ziye Yang wrote: > > > This patch is used to fix wrong operation on user input args. > > > eal_parse_args function should not operate the args passed by the > > > user. If the element in argv is generated by malloc function, > > > changing it will cause memory issues when free the args. > > > > > > Signed-off-by: Ziye Yang <ziye.yang@intel.com> > > > --- > > > lib/librte_eal/bsdapp/eal/eal.c | 2 -- > > > lib/librte_eal/linuxapp/eal/eal.c | 2 -- > > > 2 files changed, 4 deletions(-) > > > > > > diff --git a/lib/librte_eal/bsdapp/eal/eal.c > > > b/lib/librte_eal/bsdapp/eal/eal.c index 06bfd4e..0eef92d 100644 > > > --- a/lib/librte_eal/bsdapp/eal/eal.c > > > +++ b/lib/librte_eal/bsdapp/eal/eal.c > > > @@ -420,8 +420,6 @@ eal_parse_args(int argc, char **argv) > > > goto out; > > > } > > > > > > - if (optind >= 0) > > > - argv[optind-1] = prgname; > > > ret = optind-1; > > > > > > out: > > > diff --git a/lib/librte_eal/linuxapp/eal/eal.c > > > b/lib/librte_eal/linuxapp/eal/eal.c > > > index 8aafd51..ba9d1ac 100644 > > > --- a/lib/librte_eal/linuxapp/eal/eal.c > > > +++ b/lib/librte_eal/linuxapp/eal/eal.c > > > @@ -658,8 +658,6 @@ eal_parse_args(int argc, char **argv) > > > goto out; > > > } > > > > > > - if (optind >= 0) > > > - argv[optind-1] = prgname; > > > ret = optind-1; > > > > > > out: > > This is a behaviour change in DPDK. The behaviour has always been that > > after calling eal init, you can update your argv/argc values by the > > number of args parsed and then parse your app args as normal, since > > argv[0] will still point to your program name. While I agree that > > having the current behaviour may cause some problems, changing this > > behaviour may break applications that have been written to use the > existing behaviour. > > > > Since it is only the last EAL parameter arg that is modified, I think > > it would be acceptable to have the behaviour well documented and then > > expect any app to store a second copy of the pointer to be modified if > > it is needed for a subsequent free call, for example. > > > > /Bruce ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-05-11 14:07 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-05-11 5:28 [dpdk-dev] [PATCH] librte_eal: fix wrong args operation in eal_parse_args Ziye Yang 2016-05-11 11:24 ` Bruce Richardson 2016-05-11 11:51 ` Yang, Ziye 2016-05-11 12:21 ` Richardson, Bruce 2016-05-11 13:39 ` Yang, Ziye 2016-05-11 14:07 ` Richardson, Bruce
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).