* [PATCH] eal/unix: lower log level for reading files
@ 2023-10-27 8:00 David Marchand
2023-10-27 9:00 ` Morten Brørup
0 siblings, 1 reply; 5+ messages in thread
From: David Marchand @ 2023-10-27 8:00 UTC (permalink / raw)
To: dev; +Cc: ktraynor
The eal_parse_sysfs_value helper both returns an error code and logs an
error level message when something goes wrong.
On the other hand, internal users of this helper either ignore this
error code (like when trying to find out some numa information from the
Linux sysfs, or discovering some optional feature), or add their own error
logging when reading the file actually matters.
Lower this helper log messages to debug level as it provides no useful
information to final DPDK users.
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
lib/eal/unix/eal_filesystem.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/lib/eal/unix/eal_filesystem.c b/lib/eal/unix/eal_filesystem.c
index afbab9368a..282df50347 100644
--- a/lib/eal/unix/eal_filesystem.c
+++ b/lib/eal/unix/eal_filesystem.c
@@ -84,20 +84,20 @@ int eal_parse_sysfs_value(const char *filename, unsigned long *val)
char *end = NULL;
if ((f = fopen(filename, "r")) == NULL) {
- RTE_LOG(ERR, EAL, "%s(): cannot open sysfs value %s\n",
+ RTE_LOG(DEBUG, EAL, "%s(): cannot open sysfs value %s\n",
__func__, filename);
return -1;
}
if (fgets(buf, sizeof(buf), f) == NULL) {
- RTE_LOG(ERR, EAL, "%s(): cannot read sysfs value %s\n",
+ RTE_LOG(DEBUG, EAL, "%s(): cannot read sysfs value %s\n",
__func__, filename);
fclose(f);
return -1;
}
*val = strtoul(buf, &end, 0);
if ((buf[0] == '\0') || (end == NULL) || (*end != '\n')) {
- RTE_LOG(ERR, EAL, "%s(): cannot parse sysfs value %s\n",
+ RTE_LOG(DEBUG, EAL, "%s(): cannot parse sysfs value %s\n",
__func__, filename);
fclose(f);
return -1;
--
2.41.0
^ permalink raw reply [flat|nested] 5+ messages in thread* RE: [PATCH] eal/unix: lower log level for reading files
2023-10-27 8:00 [PATCH] eal/unix: lower log level for reading files David Marchand
@ 2023-10-27 9:00 ` Morten Brørup
2023-10-27 9:13 ` David Marchand
0 siblings, 1 reply; 5+ messages in thread
From: Morten Brørup @ 2023-10-27 9:00 UTC (permalink / raw)
To: David Marchand; +Cc: ktraynor, dev
> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Friday, 27 October 2023 10.01
>
> The eal_parse_sysfs_value helper both returns an error code and logs an
> error level message when something goes wrong.
> On the other hand, internal users of this helper either ignore this
> error code (like when trying to find out some numa information from the
> Linux sysfs, or discovering some optional feature), or add their own
> error
> logging when reading the file actually matters.
>
> Lower this helper log messages to debug level as it provides no useful
> information to final DPDK users.
Such assumptions seem risky.
Please add __attribute__ ((warn_unused_result)) to this function's header, to support the assumption.
Alternatively, add a "bool may_not_exist" parameter to the function to choose the relevant log level.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] eal/unix: lower log level for reading files
2023-10-27 9:00 ` Morten Brørup
@ 2023-10-27 9:13 ` David Marchand
2023-10-30 10:34 ` Thomas Monjalon
0 siblings, 1 reply; 5+ messages in thread
From: David Marchand @ 2023-10-27 9:13 UTC (permalink / raw)
To: Morten Brørup; +Cc: ktraynor, dev
On Fri, Oct 27, 2023 at 11:00 AM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > From: David Marchand [mailto:david.marchand@redhat.com]
> > Sent: Friday, 27 October 2023 10.01
> >
> > The eal_parse_sysfs_value helper both returns an error code and logs an
> > error level message when something goes wrong.
> > On the other hand, internal users of this helper either ignore this
> > error code (like when trying to find out some numa information from the
> > Linux sysfs, or discovering some optional feature), or add their own
> > error
> > logging when reading the file actually matters.
> >
> > Lower this helper log messages to debug level as it provides no useful
> > information to final DPDK users.
>
> Such assumptions seem risky.
>
> Please add __attribute__ ((warn_unused_result)) to this function's header, to support the assumption.
I can add this.
> Alternatively, add a "bool may_not_exist" parameter to the function to choose the relevant log level.
If an API update is to be considered, I would rather add some new
helpers with Windows support.
--
David Marchand
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] eal/unix: lower log level for reading files
2023-10-27 9:13 ` David Marchand
@ 2023-10-30 10:34 ` Thomas Monjalon
2023-10-30 10:45 ` Morten Brørup
0 siblings, 1 reply; 5+ messages in thread
From: Thomas Monjalon @ 2023-10-30 10:34 UTC (permalink / raw)
To: Morten Brørup, David Marchand; +Cc: ktraynor, dev
I would add "sysfs" in the title.
27/10/2023 11:13, David Marchand:
> On Fri, Oct 27, 2023 at 11:00 AM Morten Brørup <mb@smartsharesystems.com> wrote:
> >
> > > From: David Marchand [mailto:david.marchand@redhat.com]
> > > Sent: Friday, 27 October 2023 10.01
> > >
> > > The eal_parse_sysfs_value helper both returns an error code and logs an
> > > error level message when something goes wrong.
> > > On the other hand, internal users of this helper either ignore this
> > > error code (like when trying to find out some numa information from the
> > > Linux sysfs, or discovering some optional feature), or add their own
> > > error
> > > logging when reading the file actually matters.
> > >
> > > Lower this helper log messages to debug level as it provides no useful
> > > information to final DPDK users.
> >
> > Such assumptions seem risky.
> >
> > Please add __attribute__ ((warn_unused_result)) to this function's header, to support the assumption.
>
> I can add this.
Would be __rte_warn_unused_result
I'm not sure it is required to mandate checking the result.
I'm fine with or without it.
I agree with this patch because not having a sysfs entry is never critical in itself.
If the entry is required in a case, it should be handled by the caller
with a more meaningful message.
Acked-by: Thomas Monjalon <thomas@monjalon.net>
> > Alternatively, add a "bool may_not_exist" parameter to the function to choose the relevant log level.
>
> If an API update is to be considered, I would rather add some new
> helpers with Windows support.
Please don't change the API for such detail.
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] eal/unix: lower log level for reading files
2023-10-30 10:34 ` Thomas Monjalon
@ 2023-10-30 10:45 ` Morten Brørup
0 siblings, 0 replies; 5+ messages in thread
From: Morten Brørup @ 2023-10-30 10:45 UTC (permalink / raw)
To: Thomas Monjalon, David Marchand; +Cc: ktraynor, dev
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Monday, 30 October 2023 11.35
>
> I would add "sysfs" in the title.
>
> 27/10/2023 11:13, David Marchand:
> > On Fri, Oct 27, 2023 at 11:00 AM Morten Brørup
> <mb@smartsharesystems.com> wrote:
> > >
> > > > From: David Marchand [mailto:david.marchand@redhat.com]
> > > > Sent: Friday, 27 October 2023 10.01
> > > >
> > > > The eal_parse_sysfs_value helper both returns an error code and
> logs an
> > > > error level message when something goes wrong.
> > > > On the other hand, internal users of this helper either ignore
> this
> > > > error code (like when trying to find out some numa information
> from the
> > > > Linux sysfs, or discovering some optional feature), or add their
> own
> > > > error
> > > > logging when reading the file actually matters.
> > > >
> > > > Lower this helper log messages to debug level as it provides no
> useful
> > > > information to final DPDK users.
> > >
> > > Such assumptions seem risky.
> > >
> > > Please add __attribute__ ((warn_unused_result)) to this function's
> header, to support the assumption.
> >
> > I can add this.
>
> Would be __rte_warn_unused_result
>
> I'm not sure it is required to mandate checking the result.
> I'm fine with or without it.
Me too.
>
> I agree with this patch because not having a sysfs entry is never
> critical in itself.
> If the entry is required in a case, it should be handled by the caller
> with a more meaningful message.
I agree 100 %. And if all current callers do, then I consider it safe to lower the message level here. New callers are expected to follow the pattern of current callers.
>
> Acked-by: Thomas Monjalon <thomas@monjalon.net>
>
>
> > > Alternatively, add a "bool may_not_exist" parameter to the function
> to choose the relevant log level.
> >
> > If an API update is to be considered, I would rather add some new
> > helpers with Windows support.
>
> Please don't change the API for such detail.
>
I don't feel strongly about my suggested changes, so however you handle it...
Acked-by: Morten Brørup <mb@smartsharesystems.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-10-30 10:45 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-10-27 8:00 [PATCH] eal/unix: lower log level for reading files David Marchand
2023-10-27 9:00 ` Morten Brørup
2023-10-27 9:13 ` David Marchand
2023-10-30 10:34 ` Thomas Monjalon
2023-10-30 10:45 ` Morten Brørup
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).