[dpdk-dev] [PATCH 01/13] eal/log: introduce log register macro

[Jerin Jacob]

On Fri, Jun 26, 2020 at 5:46 PM David Marchand
<david.marchand at redhat.com> wrote:
>
> On Fri, Jun 26, 2020 at 2:06 PM Jerin Jacob <jerinjacobk at gmail.com> wrote:
> >
> > On Fri, Jun 26, 2020 at 5:13 PM David Marchand
> > <david.marchand at redhat.com> wrote:
> > >
> > > On Fri, Jun 26, 2020 at 1:16 PM Jerin Jacob <jerinjacobk at gmail.com> wrote:
> > > > > > Alternative is to keep variable declaration outside,
> > > > > > as David suggested, and I tend to agree that it is a
> > > > > > bit better. Macro name says 'register'. It is not
> > > > > > 'declare and register'. Also it avoids static-vs-extern
> > > > > > problem completely. The solution allows to keep the
> > > > > > variable declaration untouched and put constructor (macro)
> > > > > > at the end of fine where constructors typically reside.
> > > > >
> > > > > My only concern with that approach is that, We can not save a lot of
> > > > > code duplication
> > > > > with that scheme. ie. it is [1] vs [2]. We can change the MACRO name
> > > > > accordingly if that is a concern. Any suggestions?
> > > > >
> > > > > Let me know your preference on [1] vs [2], I will stick with for the
> > > > > next version.
> > > >
> > > > If there are no other comments, I change RTE_LOG_REGISTER to static version
> > > > and RTE_LOG_REGISTER_EXTERN for a non-static version and send the next version.
> > >
> > > - Having a macro that does more than what its name tells is inconvenient.
> >
> > I agree. What could be that name if we want to declare and register?
> > RTE_LOG_DECLARE_AND_REGISTER_EXTERN?
>
> (sorry, gmail ctrl+enter ...)
>
> Or no declaration in macro and leave code as it is.

Just to understand, Is it some general coding standard? or Just a preference.
I dont see something like that in the Linux kernel.
In this patch, http://patches.dpdk.org/patch/69681/, You are proposing
the declaration in macro.
See: RTE_TRACE_POINT_REGISTER

I dont see anything wrong with that. Do you have technical rationale
for the above rule?


>
>
> > > - Having components set log levels at init time in the macro is a bug to me.
> > > This has been worked around with
> > > rte_log_register/rte_log_register_and_pick_level but the initial
> > > problem is that rte_log_set_level* should only be called by the user.
> >
> > I agree with the below stuff, That's is not introduced by this patch.
> > It is already there.
> > Be it macro or no macro code.
> >
> > I think this patch helps to change to new scheme as it takes care of
> > changing the
> > registration part to commonplace so that we can set to the same level
> > in the future if
> > everyone agrees to it
>
> We will still expose this macro meaning that we will have an API breakage later.
> So if we go with introducing this, let's make it good from the start.

But Is everyone OK with removing the "level" from the register before
we think about
breaking the API later?. I see PMD uses multiple levels in the same
PMD for different
purposes.


>
>
> --
> David Marchand
>