Re: [PATCH v4 01/13] eal: add param register infrastructure

[Thomas Monjalon <thomas@monjalon.net>]

Hi,

11/10/2018 18:58, Kevin Laatz:
> This commit adds infrastructure to EAL that allows an application to
> register it's init function with EAL. This allows libraries to be
> initialized at the end of EAL init.
> 
> This infrastructure allows libraries that depend on EAL to be initialized
> as part of EAL init, removing circular dependency issues.

Let's try to have a clear documentation for this new infra.

> +/**
> + * @file
> + *
> + * This API introduces the ability to register callbacks with EAL.  When

You should explain when the callback is called, and what is the role of
the callback.

> + * registering a callback, the application also provides a flag as part of the
> + * struct used to register. If the flag is passed to EAL when ruuning a DPDK

What do you call a flag? Are you talking about an option to be parsed?

> + * application, the relevant callback will be called at the end of EAL init.
> + * For example, passing --telemetry will make the telemetry init be called at
> + * the end of EAl init.
> + *
> + * The register API can be used to resolve circular dependency issue between
> + * EAL and the library.

You need to explain what is the circular dependency issue.

[...]
> +struct rte_param {

Please add a global comment for this struct, explain what it represents.
> +	TAILQ_ENTRY(rte_param) next; /** The next entry in the TAILQ*/
> +	char *eal_flag;              /** The EAL flag */

eal_flag, The EAL flag
Hum...
Please use different words to explain what is a flag.
If it is something to be parsed by getopt, it should be called
an option, not a flag.

> +	char *help_text;             /** Help text for the callback */

What the help text is used for? When is it printed?

> +	rte_param_cb cb;             /** Function pointer of the callback */
> +	int enabled;                 /** Enabled flag, should be 0 by default */

What means enabled in this context?

> +};
> +
> +/**
> + * @internal Check if the passed flag is valid
> + *
> + * @param flag
> + *  The flag to be parsed

Here too, you need to be more explicit about flag meaning.

> + *
> + * @return
> + *  0 on success
> + * @return
> + *  -1 on fail
> + */
> +int
> +rte_param_parse(char *flag);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Register a function with EAL. Registering the function will enable the
> + * function to be called at the end of EAL init.
> + *
> + * @param reg_param
> + *  rte_param structure

No, this is not a helpful comment.

> + */
> +void __rte_experimental
> +rte_param_register(struct rte_param *reg_param);