From mboxrd@z Thu Jan 1 00:00:00 1970 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) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga102.jf.intel.com with ESMTP; 02 Aug 2015 12:10:04 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.15,596,1432623600"; d="scan'208";a="776339478" Received: from orsmsx107.amr.corp.intel.com ([10.22.240.5]) by orsmga002.jf.intel.com with ESMTP; 02 Aug 2015 12:10:05 -0700 Received: from orsmsx155.amr.corp.intel.com (10.22.240.21) by ORSMSX107.amr.corp.intel.com (10.22.240.5) with Microsoft SMTP Server (TLS) id 14.3.224.2; Sun, 2 Aug 2015 12:10:04 -0700 Received: from fmsmsx118.amr.corp.intel.com (10.18.116.18) by ORSMSX155.amr.corp.intel.com (10.22.240.21) with Microsoft SMTP Server (TLS) id 14.3.224.2; Sun, 2 Aug 2015 12:10:04 -0700 Received: from fmsmsx113.amr.corp.intel.com ([169.254.13.127]) by fmsmsx118.amr.corp.intel.com ([169.254.1.215]) with mapi id 14.03.0224.002; Sun, 2 Aug 2015 12:10:03 -0700 From: "Wiles, Keith" To: Thomas Monjalon Thread-Topic: [dpdk-dev] [PATCH] log:Change magic number on RTE_LOG_LEVEL to a define Thread-Index: AQHQzUcSuzbppxLeJkKsUYywZo1ZKJ35NKGA 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> In-Reply-To: <6687423.9WN7KrYfak@xps13> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.254.180.166] Content-Type: text/plain; charset="Windows-1252" Content-ID: <9E7C363190DCB64DAA399A45E63F93BF@intel.com> Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH] log:Change magic number on RTE_LOG_LEVEL to a define X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 02 Aug 2015 19:10:06 -0000 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