Re: [dpdk-dev] [PATCH 4/7] Move EAL common functions

[Ravi Kerur <rkerur@gmail.com>]

Inline <rk>

On Mon, Jan 5, 2015 at 12:38 PM, Thomas Monjalon <thomas.monjalon@6wind.com>
wrote:

> 2015-01-05 10:56, Ravi Kerur:
> > On Mon, Jan 5, 2015 at 7:59 AM, Thomas Monjalon <
> thomas.monjalon@6wind.com>
> > 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.
> >
> > <rk> 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.
>
> --
>

<rk> i have divided eal_common.c (new file which was added in v1 PATCH)
into eal_common_system.c and eal_common_runtime.c and have moved
appropriate functions into it. If file names are not appropriate or changes
are not worth doing the split, I will merge them back into
eal_common_config.c.

Thanks.

> Thomas
>