05/09/2019 08:29, Andrew Rybchenko: > 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.
Yes it is maximum. I mean even in rte_log_set_level() we don't have such word. It is a detail. > 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. I agree it can be merged in rte_log_register().
