| Message ID | 1462944501-15852-1-git-send-email-ziye.yang@intel.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Delegated to: | Thomas Monjalon |
| 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 5ADC5C1CC; Wed, 11 May 2016 07:28:29 +0200 (CEST) Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id A3621B464 for <dev@dpdk.org>; Wed, 11 May 2016 07:28:27 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga102.fm.intel.com with ESMTP; 10 May 2016 22:28:26 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,607,1455004800"; d="scan'208";a="700271526" Received: from ziyeyang-mobl2.ccr.corp.intel.com (HELO localhost.localdomain.localdomain) ([10.239.201.36]) by FMSMGA003.fm.intel.com with ESMTP; 10 May 2016 22:28:25 -0700 From: Ziye Yang <ziye.yang@intel.com> To: dev@dpdk.org Date: Wed, 11 May 2016 13:28:21 +0800 Message-Id: <1462944501-15852-1-git-send-email-ziye.yang@intel.com> X-Mailer: git-send-email 1.9.3 Subject: [dpdk-dev] [PATCH] librte_eal: fix wrong args operation in eal_parse_args 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
Ziye Yang
May 11, 2016, 5:28 a.m. UTC
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(-)
Comments
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
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
> -----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
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
> -----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
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: