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

[Thomas Monjalon <thomas.monjalon@6wind.com>]

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.

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

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

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