diff mbox series

[06/11] eal: catch invalid log level number

Message ID 20210309233116.1934666-7-thomas@monjalon.net (mailing list archive)
State Superseded, archived
Delegated to: David Marchand
Headers
Series improve options help |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Thomas Monjalon March 9, 2021, 11:31 p.m. UTC 
  The parsing check for invalid log level was not trying to catch
irrelevant numeric values.
A log level 0 or too high is now a failure in options parsing
so it can be caught early.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 lib/librte_eal/common/eal_common_options.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Bruce Richardson March 10, 2021, 12:19 p.m. UTC | #1 
On Wed, Mar 10, 2021 at 12:31:10AM +0100, Thomas Monjalon wrote:
> The parsing check for invalid log level was not trying to catch
> irrelevant numeric values.
> A log level 0 or too high is now a failure in options parsing
> so it can be caught early.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

One thing I'd note here is that our log range of 1 to 8 is a little
strange, and that it would be nice if we could accept 9 as a valid log
level too on the cmdline. Ideally 0 would also be acceptable, for all
logging off, but it's more likely that people want to up the log level than
reduce it, and 9 is a more expected max value than 8.

> ---
>  lib/librte_eal/common/eal_common_options.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
> index febc99612a..5b9ce286ff 100644
> --- a/lib/librte_eal/common/eal_common_options.c
> +++ b/lib/librte_eal/common/eal_common_options.c
> @@ -1289,7 +1289,7 @@ eal_parse_log_level(const char *arg)
>  	}
>  
>  	priority = eal_parse_log_priority(level);
> -	if (priority < 0) {
> +	if (priority <= 0 || priority > (int) RTE_LOG_MAX) {
>  		fprintf(stderr, "invalid log priority: %s\n", level);
>  		goto fail;
>  	}
Thomas Monjalon March 10, 2021, 12:33 p.m. UTC | #2 
10/03/2021 13:19, Bruce Richardson:
> On Wed, Mar 10, 2021 at 12:31:10AM +0100, Thomas Monjalon wrote:
> > The parsing check for invalid log level was not trying to catch
> > irrelevant numeric values.
> > A log level 0 or too high is now a failure in options parsing
> > so it can be caught early.
> > 
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> 
> One thing I'd note here is that our log range of 1 to 8 is a little
> strange, and that it would be nice if we could accept 9 as a valid log
> level too on the cmdline. Ideally 0 would also be acceptable, for all
> logging off, but it's more likely that people want to up the log level than
> reduce it, and 9 is a more expected max value than 8.

Why 9 is more expected?

Note that log level numbers are old-school,
now we can use symbolic names ;)
Bruce Richardson March 10, 2021, 1:26 p.m. UTC | #3 
On Wed, Mar 10, 2021 at 01:33:20PM +0100, Thomas Monjalon wrote:
> 10/03/2021 13:19, Bruce Richardson:
> > On Wed, Mar 10, 2021 at 12:31:10AM +0100, Thomas Monjalon wrote:
> > > The parsing check for invalid log level was not trying to catch
> > > irrelevant numeric values.
> > > A log level 0 or too high is now a failure in options parsing
> > > so it can be caught early.
> > > 
> > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > 
> > One thing I'd note here is that our log range of 1 to 8 is a little
> > strange, and that it would be nice if we could accept 9 as a valid log
> > level too on the cmdline. Ideally 0 would also be acceptable, for all
> > logging off, but it's more likely that people want to up the log level than
> > reduce it, and 9 is a more expected max value than 8.
> 
> Why 9 is more expected?
> 

Because a scale of 0-9 is more logical in the decimal system.
We could also generalize that any number >8 is just reduced to 8 and
we issue a warning and continue.
Thomas Monjalon March 10, 2021, 1:35 p.m. UTC | #4 
10/03/2021 14:26, Bruce Richardson:
> On Wed, Mar 10, 2021 at 01:33:20PM +0100, Thomas Monjalon wrote:
> > 10/03/2021 13:19, Bruce Richardson:
> > > On Wed, Mar 10, 2021 at 12:31:10AM +0100, Thomas Monjalon wrote:
> > > > The parsing check for invalid log level was not trying to catch
> > > > irrelevant numeric values.
> > > > A log level 0 or too high is now a failure in options parsing
> > > > so it can be caught early.
> > > > 
> > > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > > 
> > > One thing I'd note here is that our log range of 1 to 8 is a little
> > > strange, and that it would be nice if we could accept 9 as a valid log
> > > level too on the cmdline. Ideally 0 would also be acceptable, for all
> > > logging off, but it's more likely that people want to up the log level than
> > > reduce it, and 9 is a more expected max value than 8.
> > 
> > Why 9 is more expected?
> > 
> 
> Because a scale of 0-9 is more logical in the decimal system.
> We could also generalize that any number >8 is just reduced to 8 and
> we issue a warning and continue.

In this case, we should accept any high value, not limiting arbitrary
to 9, and emit a warning.
Stephen Hemminger March 10, 2021, 3:16 p.m. UTC | #5 
On Wed, 10 Mar 2021 12:19:24 +0000
Bruce Richardson <bruce.richardson@intel.com> wrote:

> On Wed, Mar 10, 2021 at 12:31:10AM +0100, Thomas Monjalon wrote:
> > The parsing check for invalid log level was not trying to catch
> > irrelevant numeric values.
> > A log level 0 or too high is now a failure in options parsing
> > so it can be caught early.
> > 
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>  
> 
> One thing I'd note here is that our log range of 1 to 8 is a little
> strange, and that it would be nice if we could accept 9 as a valid log
> level too on the cmdline. Ideally 0 would also be acceptable, for all
> logging off, but it's more likely that people want to up the log level than
> reduce it, and 9 is a more expected max value than 8.

It all goes back to syslog values.
Morten Brørup March 10, 2021, 4:35 p.m. UTC | #6 
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Wednesday, March 10, 2021 2:36 PM
> 
> 10/03/2021 14:26, Bruce Richardson:
> > On Wed, Mar 10, 2021 at 01:33:20PM +0100, Thomas Monjalon wrote:
> > > 10/03/2021 13:19, Bruce Richardson:
> > > > On Wed, Mar 10, 2021 at 12:31:10AM +0100, Thomas Monjalon wrote:
> > > > > The parsing check for invalid log level was not trying to catch
> > > > > irrelevant numeric values.
> > > > > A log level 0 or too high is now a failure in options parsing
> > > > > so it can be caught early.
> > > > >
> > > > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > > >
> > > > One thing I'd note here is that our log range of 1 to 8 is a little
> > > > strange, and that it would be nice if we could accept 9 as a valid
> log
> > > > level too on the cmdline. Ideally 0 would also be acceptable, for all
> > > > logging off, but it's more likely that people want to up the log
> level than
> > > > reduce it, and 9 is a more expected max value than 8.
> > >
> > > Why 9 is more expected?
> > >
> >
> > Because a scale of 0-9 is more logical in the decimal system.
> > We could also generalize that any number >8 is just reduced to 8 and
> > we issue a warning and continue.
> 
> In this case, we should accept any high value, not limiting arbitrary
> to 9, and emit a warning.
> 

I don't care which levels are acceptable. For reference, SYSLOG levels go from 0 to 7 (both inclusive).

-Morten
Bruce Richardson March 10, 2021, 4:52 p.m. UTC | #7 
On Wed, Mar 10, 2021 at 02:35:47PM +0100, Thomas Monjalon wrote:
> 10/03/2021 14:26, Bruce Richardson:
> > On Wed, Mar 10, 2021 at 01:33:20PM +0100, Thomas Monjalon wrote:
> > > 10/03/2021 13:19, Bruce Richardson:
> > > > On Wed, Mar 10, 2021 at 12:31:10AM +0100, Thomas Monjalon wrote:
> > > > > The parsing check for invalid log level was not trying to catch
> > > > > irrelevant numeric values.
> > > > > A log level 0 or too high is now a failure in options parsing
> > > > > so it can be caught early.
> > > > > 
> > > > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > > > 
> > > > One thing I'd note here is that our log range of 1 to 8 is a little
> > > > strange, and that it would be nice if we could accept 9 as a valid log
> > > > level too on the cmdline. Ideally 0 would also be acceptable, for all
> > > > logging off, but it's more likely that people want to up the log level than
> > > > reduce it, and 9 is a more expected max value than 8.
> > > 
> > > Why 9 is more expected?
> > > 
> > 
> > Because a scale of 0-9 is more logical in the decimal system.
> > We could also generalize that any number >8 is just reduced to 8 and
> > we issue a warning and continue.
> 
> In this case, we should accept any high value, not limiting arbitrary
> to 9, and emit a warning.
> 
+1
diff mbox series

Patch

diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index febc99612a..5b9ce286ff 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -1289,7 +1289,7 @@  eal_parse_log_level(const char *arg)
 	}
 
 	priority = eal_parse_log_priority(level);
-	if (priority < 0) {
+	if (priority <= 0 || priority > (int) RTE_LOG_MAX) {
 		fprintf(stderr, "invalid log priority: %s\n", level);
 		goto fail;
 	}