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

On Fri, Jun 26, 2020 at 6:07 PM Jerin Jacob [off-list ref] wrote:

On Fri, Jun 26, 2020 at 5:46 PM David Marchand
[off-list ref] wrote:

quoted

On Fri, Jun 26, 2020 at 2:06 PM Jerin Jacob [off-list ref] wrote:

quoted

On Fri, Jun 26, 2020 at 5:13 PM David Marchand
[off-list ref] wrote:

quoted

On Fri, Jun 26, 2020 at 1:16 PM Jerin Jacob [off-list ref] wrote:

quoted

quoted

quoted

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.

quoted

quoted

quoted

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

quoted

--
David Marchand