eal/unix: lower log level for reading files
Checks
Commit Message
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(-)
Comments
> 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.
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.
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.
> 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>
@@ -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;