[06/11] eal: catch invalid log level number
Checks
Commit Message
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
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;
> }
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 ;)
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.
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.
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.
> 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
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
@@ -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;
}