[dpdk-dev] [PATCH v4 01/13] eal: add param register infrastructure

[Thomas Monjalon]

16/10/2018 16:20, Laatz, Kevin:
> Hi Thomas,
> 
> Thanks for reviewing, see replies below.
> 
> 
> On 16/10/2018 14:42, Thomas Monjalon wrote:
> > 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.
> Are you looking for a doc file here? Or just better Doxygen comments?

Just better doxygen comments.

> >> +/**
> >> + * @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.
> I explained this below with telemetry as an example, maybe it needs to 
> be extended a little.
> >
> >> + * 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?
> Agreed. The wording was unfortunate here. Will change to 'option' to 
> make it clear it is related to getopt

OK, so you may need to change the file name, and struct/variable names.

> >> + * 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.
> I wrote the Doxygen trying to keep it generic. I can make it telemetry 
> specific if it will be clearer?

No need to be specific. You can explain the library uses EAL and
is initialized by EAL too.

> > [...]
> >> +struct rte_param {
> > Please add a global comment for this struct, explain what it represents.
> Good spot, thanks.

> >> +	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?
> The help text is currently not being used. It was added speculatively.

Please do not add things speculatively.

> >> +	rte_param_cb cb;             /** Function pointer of the callback */
> >> +	int enabled;                 /** Enabled flag, should be 0 by default */
> > What means enabled in this context?
> Enabled means that the option, for example --telemetry, was passed and 
> that we need to call the
> callback at the end of EAL init. If the option was not passed for a 
> given callback, then the callback
> will not be called.

OK, you need to explain it in the comment.

> >> +};
> >> +
> >> +/**
> >> + * @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);