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);
