From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 5E6D2E62 for ; Thu, 4 Jun 2015 18:51:31 +0200 (CEST) Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id 79162C2E31; Thu, 4 Jun 2015 16:51:30 +0000 (UTC) Received: from tfherb-2.local (vpn-56-102.rdu2.redhat.com [10.10.56.102]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t54GpSYu012627; Thu, 4 Jun 2015 12:51:28 -0400 Message-ID: <55708210.4080304@redhat.com> Date: Thu, 04 Jun 2015 12:51:28 -0400 From: Thomas F Herbert Organization: Red Hat User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Neil Horman , "Wiles, Keith" References: <1433357393-54434-1-git-send-email-keith.wiles@intel.com> <20150603171255.545e0df8@urahara> <20150604135542.GC24585@hmsreliant.think-freely.org> In-Reply-To: <20150604135542.GC24585@hmsreliant.think-freely.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 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 16:51:31 -0000 On 6/4/15 9:55 AM, Neil Horman wrote: > 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. +1. Also, I think rte_set_application_usage_hook() callback could be used by app writers for implementing usage() for a conventional " -h" like capability to print all usage including both eal and app specific args even if the eal args are not correct. This is an alternative to calling eal_init() first and bombing before printing all usage. --TFH > Neil >