| Message ID | 9d1eb8634669ef09334c7e792eb21f20b320a07b.1453150613.git.luto@kernel.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi Andy, On Mon, 18 Jan 2016 12:59:40 -0800, Andy Lutomirski wrote: > Currently they return -1 on error, which will confuse callers if > they try to interpret it as a normal negative error code. > > Signed-off-by: Andy Lutomirski <luto@kernel.org> > --- > drivers/firmware/dmi_scan.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c > index 0e08e665f715..451ac04ed18d 100644 > --- a/drivers/firmware/dmi_scan.c > +++ b/drivers/firmware/dmi_scan.c > @@ -144,7 +144,7 @@ static int __init dmi_walk_early(void (*decode)(const struct dmi_header *, > > buf = dmi_early_remap(dmi_base, orig_dmi_len); > if (buf == NULL) > - return -1; > + return -ENOMEM; > > dmi_decode_table(buf, decode, NULL); > > @@ -978,11 +978,11 @@ int dmi_walk(void (*decode)(const struct dmi_header *, void *), > u8 *buf; > > if (!dmi_available) > - return -1; > + return -ENOENT; -ENOSYS would seem more appropriate? > > buf = dmi_remap(dmi_base, dmi_len); > if (buf == NULL) > - return -1; > + return -ENOMEM; > > dmi_decode_table(buf, decode, private_data); > There is a formatted comment before dmi_walk, which says "Returns -1 when the DMI table can't be reached". This comment needs to be updated. Please also update the dmi_walk stub in include/linux/dmi.h when CONFIG_DMI isn't set. I don't think this patch should be in this series, I'd rather take it in my dmi tree and push it to Linus immediately.
On Tuesday 19 January 2016 08:54:26 Jean Delvare wrote: > > @@ -978,11 +978,11 @@ int dmi_walk(void (*decode)(const struct dmi_header *, void *), > > u8 *buf; > > > > if (!dmi_available) > > - return -1; > > + return -ENOENT; > > -ENOSYS would seem more appropriate? IIRC -ENOSYS is for non implemented syscalls.
Hi Pali, On Tue, 19 Jan 2016 09:36:33 +0100, Pali Rohár wrote: > On Tuesday 19 January 2016 08:54:26 Jean Delvare wrote: > > > @@ -978,11 +978,11 @@ int dmi_walk(void (*decode)(const struct dmi_header *, void *), > > > u8 *buf; > > > > > > if (!dmi_available) > > > - return -1; > > > + return -ENOENT; > > > > -ENOSYS would seem more appropriate? > > IIRC -ENOSYS is for non implemented syscalls. I can see a lot of -ENOSYS in include/linux/*.h returned by stubs when a specific subsystem is not included. Not related to syscalls at all. This is what lead to my suggestion.
On Tuesday 19 January 2016 10:03:03 Jean Delvare wrote: > Hi Pali, > > On Tue, 19 Jan 2016 09:36:33 +0100, Pali Rohár wrote: > > On Tuesday 19 January 2016 08:54:26 Jean Delvare wrote: > > > > @@ -978,11 +978,11 @@ int dmi_walk(void (*decode)(const struct dmi_header *, void *), > > > > u8 *buf; > > > > > > > > if (!dmi_available) > > > > - return -1; > > > > + return -ENOENT; > > > > > > -ENOSYS would seem more appropriate? > > > > IIRC -ENOSYS is for non implemented syscalls. > > I can see a lot of -ENOSYS in include/linux/*.h returned by stubs when > a specific subsystem is not included. Not related to syscalls at all. > This is what lead to my suggestion. https://lkml.org/lkml/2014/8/22/492
On Tue, 19 Jan 2016 10:07:36 +0100, Pali Rohár wrote: > On Tuesday 19 January 2016 10:03:03 Jean Delvare wrote: > > Hi Pali, > > > > On Tue, 19 Jan 2016 09:36:33 +0100, Pali Rohár wrote: > > > On Tuesday 19 January 2016 08:54:26 Jean Delvare wrote: > > > > > @@ -978,11 +978,11 @@ int dmi_walk(void (*decode)(const struct dmi_header *, void *), > > > > > u8 *buf; > > > > > > > > > > if (!dmi_available) > > > > > - return -1; > > > > > + return -ENOENT; > > > > > > > > -ENOSYS would seem more appropriate? > > > > > > IIRC -ENOSYS is for non implemented syscalls. > > > > I can see a lot of -ENOSYS in include/linux/*.h returned by stubs when > > a specific subsystem is not included. Not related to syscalls at all. > > This is what lead to my suggestion. > > https://lkml.org/lkml/2014/8/22/492 Thanks for the pointer, I wasn't aware of that. It really should be documented. No, checkpatch.pl isn't documentation. Also the commit sadly doesn't say why using ENOSYS in other contexts is considered a bad thing. What actual trouble did it cause? Are the current presumably incorrect uses of ENOSYS ultimately going to be fixed? If not, I see no point in preventing other use cases. Also what about errno(3)? It says ENOSYS is "Function not implemented" - no mention to syscalls. And glibc agrees (set errno to ENOSYS and call perror, it says "Function not implemented.") In contradiction with the proposed rule. Back to the patch under review, I would argue that ENOENT is solely for file-related operations (after all the man page says "No such file or directory"), which is why I did not like it. Then what is left to mean "function not available"? ENOTSUP? Whatever the answer is, it should also be documented and added to the checkpatch warning message (if we stick to that plan...) If you tell people "don't use this" without telling them what to use instead, each of us will come up with something different and this will lead to inconsistency. Which is worse than everybody using the same (maybe suboptimal) error code.
On Tue, Jan 19, 2016 at 1:40 AM, Jean Delvare <jdelvare@suse.de> wrote: > On Tue, 19 Jan 2016 10:07:36 +0100, Pali Rohár wrote: >> On Tuesday 19 January 2016 10:03:03 Jean Delvare wrote: >> > Hi Pali, >> > >> > On Tue, 19 Jan 2016 09:36:33 +0100, Pali Rohár wrote: >> > > On Tuesday 19 January 2016 08:54:26 Jean Delvare wrote: >> > > > > @@ -978,11 +978,11 @@ int dmi_walk(void (*decode)(const struct dmi_header *, void *), >> > > > > u8 *buf; >> > > > > >> > > > > if (!dmi_available) >> > > > > - return -1; >> > > > > + return -ENOENT; >> > > > >> > > > -ENOSYS would seem more appropriate? >> > > >> > > IIRC -ENOSYS is for non implemented syscalls. >> > >> > I can see a lot of -ENOSYS in include/linux/*.h returned by stubs when >> > a specific subsystem is not included. Not related to syscalls at all. >> > This is what lead to my suggestion. >> >> https://lkml.org/lkml/2014/8/22/492 > > Thanks for the pointer, I wasn't aware of that. > > It really should be documented. No, checkpatch.pl isn't documentation. > > Also the commit sadly doesn't say why using ENOSYS in other contexts is > considered a bad thing. What actual trouble did it cause? The trouble is that user code likes to assume that, when a syscall returns -ENOSYS, that syscall isn't implemented. Letting ENOSYS leak out to userspace via a syscall that *is* implemented can confused things. > > Are the current presumably incorrect uses of ENOSYS ultimately going to > be fixed? If not, I see no point in preventing other use cases. We at least want to prevent it from newly introduced syscalls. I'll try to clean up the docs. --Andy -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c index 0e08e665f715..451ac04ed18d 100644 --- a/drivers/firmware/dmi_scan.c +++ b/drivers/firmware/dmi_scan.c @@ -144,7 +144,7 @@ static int __init dmi_walk_early(void (*decode)(const struct dmi_header *, buf = dmi_early_remap(dmi_base, orig_dmi_len); if (buf == NULL) - return -1; + return -ENOMEM; dmi_decode_table(buf, decode, NULL); @@ -978,11 +978,11 @@ int dmi_walk(void (*decode)(const struct dmi_header *, void *), u8 *buf; if (!dmi_available) - return -1; + return -ENOENT; buf = dmi_remap(dmi_base, dmi_len); if (buf == NULL) - return -1; + return -ENOMEM; dmi_decode_table(buf, decode, private_data);
Currently they return -1 on error, which will confuse callers if they try to interpret it as a normal negative error code. Signed-off-by: Andy Lutomirski <luto@kernel.org> --- drivers/firmware/dmi_scan.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)