From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Monjalon Subject: Re: [PATCH 4/7] Move EAL common functions Date: Mon, 05 Jan 2015 21:38:27 +0100 Message-ID: <3095360.cfZ4UIMKPn@xps13> References: <1419521597-31978-1-git-send-email-rkerur@gmail.com> <20293089.lIc3yRvdMZ@xps13> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7Bit Cc: dev-VfR2kkLFssw@public.gmane.org To: Ravi Kerur Return-path: In-Reply-To: List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org Sender: "dev" 2015-01-05 10:56, Ravi Kerur: > On Mon, Jan 5, 2015 at 7:59 AM, Thomas Monjalon > wrote: > > 2014-12-25 10:33, Ravi Kerur: > > > Move common functions in eal.c to librte_eal/common directory. > > [...] > > > lib/librte_eal/common/eal_common.c | 328 ++++++++++++++++++++++++++++++++++ > > > lib/librte_eal/common/eal_externs.h | 42 +++++ > > > > I don't agree with these new files. > > We must try to keep a semantic organization. The file eal_common_options.c > > would be better for option-related functions. > > Maybe that the split between system config, runtime config and internal > > config > > must be reworked. > > > > By the way, it would be nice to avoid extern variables. > > I have taken care of your comments and will generate v4 patch. Please do not forget v4 word and changelog when sending patches. Check http://dpdk.org/dev#send > Currently I have moved common functions in eal.c into > "eal_common_system_options.c" file. Are you suggesting that we further > divide "eal_common_options.c" and "eal_common_system_options.c(new file > added)" into 3 separate files i.e. > > eal_common_system_options.c > eal_common_runtime_options.c > eal_common_internal_options.c You are already doing big changes. So let's iterate with existing files and avoid creating new ones. eal_common_options.c must be kept. But if some code is not really related to runtime options, we could consider adding a new file eal_common_config.c, not sure about this one. -- Thomas