On 8/2/15, 12:15 PM, "Thomas Monjalon" <thomas.monjalon at 6wind.com> wrote:
>2015-06-06 19:04, Keith Wiles:
>> --- a/config/common_bsdapp
>> +++ b/config/common_bsdapp
>> @@ -93,12 +93,18 @@ CONFIG_RTE_MAX_NUMA_NODES=8
>> CONFIG_RTE_MAX_MEMSEG=256
>> CONFIG_RTE_MAX_MEMZONE=2560
>> CONFIG_RTE_MAX_TAILQ=32
>> -CONFIG_RTE_LOG_LEVEL=8
>> CONFIG_RTE_LOG_HISTORY=256
>> CONFIG_RTE_EAL_ALLOW_INV_SOCKET_ID=n
>> CONFIG_RTE_EAL_ALWAYS_PANIC_ON_ERROR=n
>>
>> #
>> +# Log level use: RTE_LOG_XXX
>> +# XXX = NOOP, EMERG, ALERT, CRIT, ERR, WARNING, NOTICE, INFO or DEBUG
>> +# Look in rte_log.h for others if any.
>> +#
>
>I think this comment is useless.
I do not think the comment is useless as some may not understand what
values the Log level can be set too in the future. Not commenting the
change would be a problem IMO. This is also why the line was moved.
>
>> +CONFIG_RTE_LOG_LEVEL=RTE_LOG_DEBUG
>
>Yes, easier to read.
>Please do not move line without good reason. It was more logic to see it
>along
>with LOG_HISTORY.
Moving the line was for the comment and now it is a enum value instead of
a magic number. Magic numbers are bad right? Adding a comment to help the
user set this value is always reasonable IMO unless the comment is not
correct, is this the case?
>
>> --- a/lib/librte_eal/common/eal_common_log.c
>> +++ b/lib/librte_eal/common/eal_common_log.c
>> @@ -82,7 +82,7 @@ static struct log_history_list log_history;
>> /* global log structure */
>> struct rte_logs rte_logs = {
>> .type = ~0,
>> - .level = RTE_LOG_DEBUG,
>> + .level = RTE_LOG_LEVEL,
>> .file = NULL,
>> };
>
>OK, more consistent.
>It was set to RTE_LOG_LEVEL later anyway.
>(this comment would be useful in the commit message)
>
>> /* Can't use 0, as it gives compiler warnings */
>> -#define RTE_LOG_EMERG 1U /**< System is unusable. */
>> -#define RTE_LOG_ALERT 2U /**< Action must be taken immediately. */
>> -#define RTE_LOG_CRIT 3U /**< Critical conditions. */
>> -#define RTE_LOG_ERR 4U /**< Error conditions. */
>> -#define RTE_LOG_WARNING 5U /**< Warning conditions. */
>> -#define RTE_LOG_NOTICE 6U /**< Normal but significant condition. */
>> -#define RTE_LOG_INFO 7U /**< Informational. */
>> -#define RTE_LOG_DEBUG 8U /**< Debug-level messages. */
>> +enum {
>> + RTE_LOG_NOOP = 0, /**< Noop not used (zero entry) */
>
>NOOP is useless: EMERG may be = 1
Does it really matter if I used RTE_LOG_NOOP, just to make sure someone
did not try to use zero here. Instead of setting the RTE_LOG_EMERG=1, I
can change it to be the way you suggest, but I think it does not hurt
anything does it?
>
>> + RTE_LOG_EMERG, /**< System is unusable. */
>> + RTE_LOG_ALERT, /**< Action must be taken immediately. */
>> + RTE_LOG_CRIT, /**< Critical conditions. */
>> + RTE_LOG_ERR, /**< Error conditions. */
>> + RTE_LOG_WARNING, /**< Warning conditions. */
>> + RTE_LOG_NOTICE, /**< Normal but significant condition. */
>> + RTE_LOG_INFO, /**< Informational. */
>> + RTE_LOG_DEBUG /**< Debug-level messages. */
>> +};
>
>What is the benefit of this change?
The change is to use a enum in place of using magic numbers, plus you get
the benefit of seeing the enum name in the debugger instead of a number.
It makes the code more readable IMHO.
>
>
To me the code is fine and the only change would be the RTE_LOG_NOOP being
remove and RTE_LOG_EMERG=1.
?
Regards,
++Keith
Intel Corporation
