On Thu, Sep 5, 2019 at 8:29 AM Andrew Rybchenko <arybche...@solarflare.com> wrote: > > On 9/4/19 11:44 PM, Thomas Monjalon wrote: > > 04/09/2019 21:58, Andrew Rybchenko: > > On September 4, 2019 22:42:12 Thomas Monjalon <tho...@monjalon.net> wrote: > > 04/09/2019 21:21, Andrew Rybchenko: > > On 9/4/19 8:45 PM, Thomas Monjalon wrote: > > 03/09/2019 10:47, Ferruh Yigit: > > On 9/3/2019 9:06 AM, David Marchand wrote: > > On Mon, Sep 2, 2019 at 4:29 PM Ferruh Yigit <ferruh.yi...@intel.com> wrote: > > On 8/19/2019 12:41 PM, David Marchand wrote: > > The function rte_log_register_type_and_pick_level() fills a gap for > dynamically loaded code (especially drivers) who would not pick up > the log level passed at startup. > > > Let's promote it to stable and export it for use by drivers via > a wrapper. > > > Signed-off-by: David Marchand <david.march...@redhat.com> > --- > > [...] > > /** > - * @warning > - * @b EXPERIMENTAL: this API may change without prior notice > - * > * Register a dynamic log type and try to pick its level from EAL options > > [...] > > -__rte_experimental > int rte_log_register_type_and_pick_level(const char *name, uint32_t > level_def); > > +1 to remove experimental from the API. > > I am not sure about this function API. > Why we combined register and level setting in one function? > > See [1] > > [1] http://git.dpdk.org/dpdk/commit/?id=b22e77c026 > > Sorry, it does not explain why we mix both operations in one function. > > Exactly to have one function instead of repeating two function calls > everywhere. Log level should be set when log type is registered. Yes, it is > possible to factor out a function just to pick log level, but I'm not sure > it makes sense separately. > > In fact may be it makes sense to substitute just register with this one > (I.e. remove simple register from piblic API and do not highlight that > level is picked up in function name). > > Given that we use it in a macro, we could keep two separate functions > for logtype register and minimum log level (note that "minimum" is > currently a missing word in these functions). > Anyway, we will always use the single macro in our libraries. > > > I don't understand why "minimum" is missing. It assigns level specified > for the pattern if it matches, or use level_def if no match. No level > comparison, last match wins. If "minimum" refers to minimum logged > level, it matches rte_log_set_level() which has no "minimum" as well > in neither name nor description. Moreover, EMERG is smaller than > DEBUG and if we treat log levels as numbers, it sets maximum level > which is logged. > > There are two usages right now without a macro, but it is not that > important. What I'm trying to say is that rte_log_register() plus setting > default after register is too error prone. It is the source of the bug > which we tried to workaround introducing the function. > > If user sets log level (using eal command-line options), no special > efforts should be required to assign it on register. So, I think register > should always do it. The function name is not ideal since it is highlights > details which are not really interesting. It is logging support details that > log levels may be set before log types registration. > > if we accept DPDK-wide default, it makes level_def parameter > unnecessary and the functionality to pick level may be simply > moved to rte_log_register() (using helper internal function) and > I see no good reasons why we really need type-specific defaults.
Well this would require to touch quite some components that are not aligned, but I can't disagree with you :-). I'll prepare a v2 and identify which drivers are not ready for this. I also have some additional changes in mind. This should prevent the log register from failing (and no need for the "fallback" stuff then). -- David Marchand
