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?
+/**
+ * @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
+ * 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?
[...]
+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.
+ 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.
+};
+
+/**
+ * @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);
