From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Wiles, Keith" Subject: Re: [PATCH] log:Change magic number on RTE_LOG_LEVEL to a define Date: Sun, 2 Aug 2015 19:10:02 +0000 Message-ID: References: <1433635446-78275-1-git-send-email-keith.wiles@intel.com> <1433635446-78275-2-git-send-email-keith.wiles@intel.com> <6687423.9WN7KrYfak@xps13> Mime-Version: 1.0 Content-Type: text/plain; charset="Windows-1252" Content-Transfer-Encoding: quoted-printable Cc: "dev@dpdk.org" To: Thomas Monjalon Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 0B4C4C3E6 for ; Sun, 2 Aug 2015 21:10:05 +0200 (CEST) In-Reply-To: <6687423.9WN7KrYfak@xps13> Content-Language: en-US Content-ID: <9E7C363190DCB64DAA399A45E63F93BF@intel.com> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 8/2/15, 12:15 PM, "Thomas Monjalon" 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=3D8 >> CONFIG_RTE_MAX_MEMSEG=3D256 >> CONFIG_RTE_MAX_MEMZONE=3D2560 >> CONFIG_RTE_MAX_TAILQ=3D32 >> -CONFIG_RTE_LOG_LEVEL=3D8 >> CONFIG_RTE_LOG_HISTORY=3D256 >> CONFIG_RTE_EAL_ALLOW_INV_SOCKET_ID=3Dn >> CONFIG_RTE_EAL_ALWAYS_PANIC_ON_ERROR=3Dn >> =20 >> # >> +# Log level use: RTE_LOG_XXX >> +# XXX =3D NOOP, EMERG, ALERT, CRIT, ERR, WARNING, NOTICE, INFO or DEB= UG >> +# 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=3DRTE_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 =3D { >> .type =3D ~0, >> - .level =3D RTE_LOG_DEBUG, >> + .level =3D RTE_LOG_LEVEL, >> .file =3D 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 =3D 0, /**< Noop not used (zero entry) */ > >NOOP is useless: EMERG may be =3D 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=3D1, 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=3D1. =8B=20 Regards, ++Keith Intel Corporation