From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58]) by dpdk.org (Postfix) with ESMTP id DFAD9C346 for ; Thu, 4 Jun 2015 15:55:56 +0200 (CEST) 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 ) id 1Z0Vca-0003zF-Jb; Thu, 04 Jun 2015 09:55:54 -0400 Date: Thu, 4 Jun 2015 09:55:42 -0400 From: Neil Horman To: "Wiles, Keith" Message-ID: <20150604135542.GC24585@hmsreliant.think-freely.org> References: <1433357393-54434-1-git-send-email-keith.wiles@intel.com> <20150603171255.545e0df8@urahara> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) X-Spam-Score: -2.9 (--) X-Spam-Status: No Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [RFC PATCH] eal:Add new API for parsing args at rte_eal_init time 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: Thu, 04 Jun 2015 13:55:57 -0000 On Thu, Jun 04, 2015 at 11:50:33AM +0000, Wiles, Keith wrote: > Hi Stephen > > On 6/3/15, 7:12 PM, "Stephen Hemminger" wrote: > > >On Wed, 3 Jun 2015 13:49:53 -0500 > >Keith Wiles wrote: > > > >> +/* Launch threads, called at application init() and parse app args. */ > >> +int > >> +rte_eal_init_parse(int argc, char **argv, > >> + int (*parse)(int, char **)) > >> +{ > >> + int ret; > >> + > >> + ret = rte_eal_init(argc, argv); > >> + if ((ret >= 0) && (parse != NULL)) { > >> + argc -= ret; > >> + argv += ret; > > > >This won't work C is call by value. > > I tested this routine with Pktgen (again), which has a number of > application options and it appears to work correctly. Can you explain why > this will not work? > > Regards, > ++Keith > > > > Stephen was thinking that your intent was to have argc, and argv modified at the call site of this function (i.e. if you called rte_eal_init_parse from main(), then after the call to rte_ela_init_parse, argc would be reduced by ret and argv would point forward in memory ret bytes in the main function, but they wont. It doesn't matter though, because you return ret, so the caller can do that movement themselves. As you note, it works. Note that if it was your intention to have argc and argv modified at the call site, then Stephen is right and this is broken, you need to modify the prototype to be: int rte_eal_init_parse(int *argc, char ***argv) and do a dereference when modifying the parameters so the change is seen at the call site. That said, I'm not sure theres much value in adding this to the API. For one, it implies that dpdk arguments need to come first on the command line. While all the example applications do that, theres no requirement that they do so, and this function silently implies that they have to, so any existing applications in the wild that violate that assumption are enjoined from using this It also doesn't really save any code. If we pick an example app (I'll us l2fwd-jobstats), We currently have this: /* init EAL */ ret = rte_eal_init(argc, argv); if (ret < 0) rte_exit(EXIT_FAILURE, "Invalid EAL arguments\n"); argc -= ret; argv += ret; /* parse application arguments (after the EAL ones) */ ret = l2fwd_parse_args(argc, argv); if (ret < 0) rte_exit(EXIT_FAILURE, "Invalid L2FWD arguments\n"); With your new API we would get this: ret = rte_eal_init_parse(argc, argv, l2fwd_parse_args) if (ret < 0) rte_exit(EXIT_FAILURE, "Invalid arguments - not sure what\n"); Its definately 5 fewer lines of source, but it doesn't save any execution instructions, and for the effort of that, you loose the ability to determine if it was a DPDK argument or an application argument that failed. Its not a bad addition, I'm just not sure its worth having to take on the additional API surface to include. I'd be more supportive if you could enhance the function to allow the previously mentioned before/after flexibiilty. Then we could just deprecate rte_eal_init as an API call entirely, and use this instead. Neil