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

[Jerin Jacob <jerinjacobk@gmail.com>]

On Fri, Jun 26, 2020 at 6:07 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>
> On Fri, Jun 26, 2020 at 5:46 PM David Marchand
> <david.marchand@redhat.com> wrote:
> >
> > On Fri, Jun 26, 2020 at 2:06 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > >
> > > On Fri, Jun 26, 2020 at 5:13 PM David Marchand
> > > <david.marchand@redhat.com> wrote:
> > > >
> > > > On Fri, Jun 26, 2020 at 1:16 PM Jerin Jacob <jerinjacobk@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?

David,

Waiting for your feedback on this to send the next version.
If you have a strong preference for keeping declaration out macro then
I will respin the v2 based on that. Let me know.



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