Re: [PATCH] log:Change magic number on RTE_LOG_LEVEL to a define

["Wiles, Keith" <keith.wiles@intel.com>]

On 8/2/15, 2:10 PM, "dev on behalf of Wiles, Keith" <dev-bounces@dpdk.org
on behalf of keith.wiles@intel.com> wrote:

>On 8/2/15, 12:15 PM, "Thomas Monjalon" <thomas.monjalon@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.

Maybe moving LOG_HISTORY with LOG_LEVEL would have been a better option.
>
>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
>
>
>
>


— 
Regards,
++Keith
Intel Corporation