[dpdk-dev] [PATCH v8 1/9] eal: move OS common functions to single file

[Tal Shnaiderman]

> Subject: Re: [dpdk-dev] [PATCH v8 1/9] eal: move OS common functions to
> single file
> 
> Can we reduce the scope in the title?
> This patch is about EAL config in general, so I would reword it to:
> 	eal: move OS common config objects
> 

Agree, will change in v9.

> 22/06/2020 09:55, talshn at mellanox.com:
> > +#include <rte_os.h>
> > +
> > +#include <eal_private.h>
> > +#include <eal_memcfg.h>
> 
> Local files, like eal_*, are usually included with "".
> 
> [...]
> > +/* Allow the application to print its usage message too if set */
> > +static rte_usage_hook_t rte_application_usage_hook;
> 
> This is not config-related.
> Could it be in a separate patch?
> Moved to lib/librte_eal/common/eal_common_options.c?
> 

Agree, will change in v9.

> [...]
> > +void
> > +rte_eal_set_runtime_dir(char *run_dir, size_t size) {
> > +	strncpy(runtime_dir, run_dir, size); }
> 
> This function should be private to EAL, no rte_ prefix.
> 
> [...]
> > +/* Return a pointer to theinternal configuration structure */
> 
> Space missing
> 
> > +struct internal_config *
> > +rte_eal_get_internal_configuration(void)
> > +{
> > +	return &internal_config;
> > +}
> 
> This function should be private to EAL, no rte_ prefix.
>

When creating the new functions I based the pattern on the already existing function
rte_eal_get_configuration() which encapsulates the formally global rte_config.

Do we want to remove the rte_ prefix from this function as well?
 
> [...]
> > --- a/lib/librte_eal/include/rte_eal.h
> > +++ b/lib/librte_eal/include/rte_eal.h
> > +void
> > +rte_eal_set_runtime_dir(char *run_dir, size_t size);
> > +
> > +void
> > +rte_eal_config_remap(void *mem_cfg_addr);
> > +
> > +struct internal_config *
> > +rte_eal_get_internal_configuration(void);
> > +
> > +rte_usage_hook_t *
> > +rte_eal_get_application_usage_hook(void);
> 
> If some new function must be added in the API, they must be clearly
> documented with doxygen.
> But as part of this refactoring, I guess we should have zero new API.
> Probably that those 4 functions are candidate to internal header files.
>