Re: [PATCH v2] log:Change magic number on RTE_LOG_LEVEL to an enum name

[Stephen Hemminger <stephen@networkplumber.org>]

On Sun,  2 Aug 2015 16:40:24 -0500
Keith Wiles <keith.wiles@intel.com> wrote:

> Config files used RTE_LOG_LEVEL=8 to set log level to DEBUG. Using
> a the RTE_LOG_XXXX is easier to maintain.
> 
> Converted the RTE_LOG_XXXX define into a enum of values with
> the same names to reduce maintaining the define values.
> 
> The change is to use an enum in place of a magic number, plus
> we get the benefit of seeing the enum name in the debugger
> instead of a number.
> 
> The rte_logs was set after options parsing, defaulting to
> RTE_LOG_LEVEL, and it is now initialized at RTE_LOG_LEVEL
> without behavioral change.
> 
> Signed-off-by: Keith Wiles <keith.wiles@intel.com>
> ---
>  config/common_bsdapp                    |  6 ++++--
>  config/common_linuxapp                  |  6 ++++--
>  lib/librte_eal/common/eal_common_log.c  |  4 ++--
>  lib/librte_eal/common/include/rte_log.h | 18 ++++++++++--------
>  4 files changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/config/common_bsdapp b/config/common_bsdapp
> index 2c6eb40..b2e9462 100644
> --- a/config/common_bsdapp
> +++ b/config/common_bsdapp
> @@ -102,12 +102,14 @@ 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
>  CONFIG_RTE_MALLOC_DEBUG=n
>  
> +# RTE_LOG_XXX = DEBUG, INFO, NOTICE, WARNING or ERR
> +CONFIG_RTE_LOG_LEVEL=RTE_LOG_DEBUG
> +CONFIG_RTE_LOG_HISTORY=256
> +
>  #
>  # FreeBSD contiguous memory driver settings
>  #
> diff --git a/config/common_linuxapp b/config/common_linuxapp
> index bda9a63..eb0f659 100644
> --- a/config/common_linuxapp
> +++ b/config/common_linuxapp
> @@ -102,8 +102,6 @@ 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_LIBEAL_USE_HPET=n
>  CONFIG_RTE_EAL_ALLOW_INV_SOCKET_ID=n
>  CONFIG_RTE_EAL_ALWAYS_PANIC_ON_ERROR=n
> @@ -111,6 +109,10 @@ CONFIG_RTE_EAL_IGB_UIO=y
>  CONFIG_RTE_EAL_VFIO=y
>  CONFIG_RTE_MALLOC_DEBUG=n
>  
> +# RTE_LOG_XXX = DEBUG, INFO, NOTICE, WARNING or ERR
> +CONFIG_RTE_LOG_LEVEL=RTE_LOG_DEBUG
> +CONFIG_RTE_LOG_HISTORY=256
> +
>  #
>  # Special configurations in PCI Config Space for high performance
>  #
> diff --git a/lib/librte_eal/common/eal_common_log.c b/lib/librte_eal/common/eal_common_log.c
> index 1ae8de7..6ed6743 100644
> --- 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,
>  };
>  
> @@ -93,7 +93,7 @@ static int history_enabled = 1;
>  
>  /**
>   * This global structure stores some informations about the message
> - * that is currently beeing processed by one lcore
> + * that is currently being processed by one lcore
>   */
>  struct log_cur_msg {
>  	uint32_t loglevel; /**< log level - see rte_log.h */
> diff --git a/lib/librte_eal/common/include/rte_log.h b/lib/librte_eal/common/include/rte_log.h
> index 24a55cc..be75a45 100644
> --- a/lib/librte_eal/common/include/rte_log.h
> +++ b/lib/librte_eal/common/include/rte_log.h
> @@ -89,14 +89,16 @@ extern struct rte_logs rte_logs;
>  #define RTE_LOGTYPE_USER8   0x80000000 /**< User-defined log type 8. */
>  
>  /* 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_EMERG=1,    /**< 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.             */
> +};
>  

This is technically a source API change. There are cases where
you could break build of existing code that was expecting #define's
(but I doubt anyone would notice0.