* [PATCH] livepatch: core: Return ENOTSUPP instead of ENOSYS
@ 2019-01-26 19:26 Alice Ferrazzi
2019-01-28 19:49 ` Joe Lawrence
0 siblings, 1 reply; 6+ messages in thread
From: Alice Ferrazzi @ 2019-01-26 19:26 UTC (permalink / raw)
To: jpoimboe, jeyu, jikos, mbenes, pmladek, live-patching, linux-kernel
Cc: Alice Ferrazzi, Alice Ferrazzi
This patch fixes a checkpatch warning:
WARNING: ENOSYS means 'invalid syscall nr' and nothing else
Signed-off-by: Alice Ferrazzi <alice.ferrazzi@miraclelinux.com>
---
kernel/livepatch/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 5b77a7314e01..eea6b94fef89 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -897,7 +897,7 @@ int klp_register_patch(struct klp_patch *patch)
if (!klp_have_reliable_stack()) {
pr_err("This architecture doesn't have support for the livepatch consistency model.\n");
- return -ENOSYS;
+ return -ENOTSUPP;
}
return klp_init_patch(patch);
--
2.19.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] livepatch: core: Return ENOTSUPP instead of ENOSYS
2019-01-26 19:26 [PATCH] livepatch: core: Return ENOTSUPP instead of ENOSYS Alice Ferrazzi
@ 2019-01-28 19:49 ` Joe Lawrence
2019-01-29 16:50 ` Josh Poimboeuf
0 siblings, 1 reply; 6+ messages in thread
From: Joe Lawrence @ 2019-01-28 19:49 UTC (permalink / raw)
To: Alice Ferrazzi
Cc: jpoimboe, jeyu, jikos, mbenes, pmladek, live-patching,
linux-kernel, Alice Ferrazzi
On Sun, Jan 27, 2019 at 04:26:30AM +0900, Alice Ferrazzi wrote:
> This patch fixes a checkpatch warning:
> WARNING: ENOSYS means 'invalid syscall nr' and nothing else
>
> Signed-off-by: Alice Ferrazzi <alice.ferrazzi@miraclelinux.com>
> ---
> kernel/livepatch/core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 5b77a7314e01..eea6b94fef89 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -897,7 +897,7 @@ int klp_register_patch(struct klp_patch *patch)
>
> if (!klp_have_reliable_stack()) {
> pr_err("This architecture doesn't have support for the livepatch consistency model.\n");
> - return -ENOSYS;
> + return -ENOTSUPP;
> }
>
> return klp_init_patch(patch);
> --
> 2.19.2
>
Hi Alice,
Patches should be based off the upstream livepatching tree, found here:
git://git.kernel.org/pub/scm/linux/kernel/git/livepatching/livepatching.git
and in this case, the for-next branch, which holds patches that have
already been queued up for the next release. This one:
958ef1e39d24 ("livepatch: Simplify API by removing registration step")
has moved the code in question from klp_register_patch() to
klp_enable_patch().
As far as the change itself, I don't have strong opinion about it
either way.
On the one hand, there is the checkpatch warning and -ENOTSUPP reads
more intuitively than -ENOSYS.
However, the current pattern seems to be more prevelent in the kernel.
I wonder if the checkpatch warning would be better specified for return
values that are actually passed back to userspace.
Also, klp_register_patch(), now klp_enable_patch(), is exported for
module use, though I don't believe anyone (samples / tests / kpatch /
kgraft?) is inspecting which error value is returned.
I would defer to whichever convention the maintainers prefer here.
-- Joe
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] livepatch: core: Return ENOTSUPP instead of ENOSYS
2019-01-28 19:49 ` Joe Lawrence
@ 2019-01-29 16:50 ` Josh Poimboeuf
2019-01-30 12:41 ` Petr Mladek
0 siblings, 1 reply; 6+ messages in thread
From: Josh Poimboeuf @ 2019-01-29 16:50 UTC (permalink / raw)
To: Joe Lawrence
Cc: Alice Ferrazzi, jeyu, jikos, mbenes, pmladek, live-patching,
linux-kernel, Alice Ferrazzi
On Mon, Jan 28, 2019 at 02:49:43PM -0500, Joe Lawrence wrote:
> On Sun, Jan 27, 2019 at 04:26:30AM +0900, Alice Ferrazzi wrote:
> > This patch fixes a checkpatch warning:
> > WARNING: ENOSYS means 'invalid syscall nr' and nothing else
> >
> > Signed-off-by: Alice Ferrazzi <alice.ferrazzi@miraclelinux.com>
> > ---
> > kernel/livepatch/core.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index 5b77a7314e01..eea6b94fef89 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -897,7 +897,7 @@ int klp_register_patch(struct klp_patch *patch)
> >
> > if (!klp_have_reliable_stack()) {
> > pr_err("This architecture doesn't have support for the livepatch consistency model.\n");
> > - return -ENOSYS;
> > + return -ENOTSUPP;
> > }
> >
> > return klp_init_patch(patch);
> > --
> > 2.19.2
> >
>
> Hi Alice,
>
> Patches should be based off the upstream livepatching tree, found here:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/livepatching/livepatching.git
>
> and in this case, the for-next branch, which holds patches that have
> already been queued up for the next release. This one:
>
> 958ef1e39d24 ("livepatch: Simplify API by removing registration step")
>
> has moved the code in question from klp_register_patch() to
> klp_enable_patch().
>
>
> As far as the change itself, I don't have strong opinion about it
> either way.
>
> On the one hand, there is the checkpatch warning and -ENOTSUPP reads
> more intuitively than -ENOSYS.
>
> However, the current pattern seems to be more prevelent in the kernel.
> I wonder if the checkpatch warning would be better specified for return
> values that are actually passed back to userspace.
>
> Also, klp_register_patch(), now klp_enable_patch(), is exported for
> module use, though I don't believe anyone (samples / tests / kpatch /
> kgraft?) is inspecting which error value is returned.
>
> I would defer to whichever convention the maintainers prefer here.
Based on the commit description from 91c9afaf97ee ("checkpatch.pl: new
instances of ENOSYS are errors"), it sounds like there was a decision at
Kernel Summit to limit ENOSYS to mean "bad syscall" and nothing else.
So I'm ok with this change, though the patch description should have a
little more background on why it's being done -- checkpatch.pl alone
isn't a good justification because some checkpatch warnings are best
taken with a grain of salt.
--
Josh
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] livepatch: core: Return ENOTSUPP instead of ENOSYS
2019-01-29 16:50 ` Josh Poimboeuf
@ 2019-01-30 12:41 ` Petr Mladek
2019-01-30 13:00 ` alicef
2019-01-30 13:10 ` Josh Poimboeuf
0 siblings, 2 replies; 6+ messages in thread
From: Petr Mladek @ 2019-01-30 12:41 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Joe Lawrence, Alice Ferrazzi, jeyu, jikos, mbenes, live-patching,
linux-kernel, Alice Ferrazzi
On Tue 2019-01-29 10:50:54, Josh Poimboeuf wrote:
> On Mon, Jan 28, 2019 at 02:49:43PM -0500, Joe Lawrence wrote:
> > On Sun, Jan 27, 2019 at 04:26:30AM +0900, Alice Ferrazzi wrote:
> > > This patch fixes a checkpatch warning:
> > > WARNING: ENOSYS means 'invalid syscall nr' and nothing else
> > >
> > > Signed-off-by: Alice Ferrazzi <alice.ferrazzi@miraclelinux.com>
> > > ---
> > > kernel/livepatch/core.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > > index 5b77a7314e01..eea6b94fef89 100644
> > > --- a/kernel/livepatch/core.c
> > > +++ b/kernel/livepatch/core.c
> > > @@ -897,7 +897,7 @@ int klp_register_patch(struct klp_patch *patch)
> > >
> > > if (!klp_have_reliable_stack()) {
> > > pr_err("This architecture doesn't have support for the livepatch consistency model.\n");
> > > - return -ENOSYS;
> > > + return -ENOTSUPP;
> > > }
> > >
> > > return klp_init_patch(patch);
> > > --
> > > 2.19.2
> > >
> >
> > Hi Alice,
> >
> > Patches should be based off the upstream livepatching tree, found here:
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/livepatching/livepatching.git
> >
> > and in this case, the for-next branch, which holds patches that have
> > already been queued up for the next release. This one:
> >
> > 958ef1e39d24 ("livepatch: Simplify API by removing registration step")
> >
> > has moved the code in question from klp_register_patch() to
> > klp_enable_patch().
> >
> >
> > As far as the change itself, I don't have strong opinion about it
> > either way.
> >
> > On the one hand, there is the checkpatch warning and -ENOTSUPP reads
> > more intuitively than -ENOSYS.
> >
> > However, the current pattern seems to be more prevelent in the kernel.
> > I wonder if the checkpatch warning would be better specified for return
> > values that are actually passed back to userspace.
> >
> > Also, klp_register_patch(), now klp_enable_patch(), is exported for
> > module use, though I don't believe anyone (samples / tests / kpatch /
> > kgraft?) is inspecting which error value is returned.
> >
> > I would defer to whichever convention the maintainers prefer here.
>
> Based on the commit description from 91c9afaf97ee ("checkpatch.pl: new
> instances of ENOSYS are errors"), it sounds like there was a decision at
> Kernel Summit to limit ENOSYS to mean "bad syscall" and nothing else.
Hmm, the error code is passed to the syscall, for example:
+ SYSCALL_DEFINE3(init_module
+ load_module()
+ do_init_module()
+ do_one_initcall(mod->init);
I am not sure if we are allowed to return -ENOTSUPP (-524).
It is defined in the internal include/linux/errno.h. There
is the following commnent:
/*
* These should never be seen by user programs...
I tried to find a better alternative and found:
#define EOPNOTSUPP 95 /* Operation not supported on transport endpoint */
There is the following note in man errno:
ENOTSUP Operation not supported (POSIX.1)
EOPNOTSUPP Operation not supported on socket (POSIX.1)
(ENOTSUP and EOPNOTSUPP have the same value
on Linux, but according to POSIX.1 these error
values should be distinct.)
And it looks that -EOPNOTSUPP is used widely in many subsystes (not
only network).
Best Regards,
Petr
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] livepatch: core: Return ENOTSUPP instead of ENOSYS
2019-01-30 12:41 ` Petr Mladek
@ 2019-01-30 13:00 ` alicef
2019-01-30 13:10 ` Josh Poimboeuf
1 sibling, 0 replies; 6+ messages in thread
From: alicef @ 2019-01-30 13:00 UTC (permalink / raw)
To: Petr Mladek
Cc: Josh Poimboeuf, Joe Lawrence, jeyu, jikos, mbenes, live-patching,
linux-kernel, Alice Ferrazzi, live-patching-owner
On 2019-01-30 21:41, Petr Mladek wrote:
> On Tue 2019-01-29 10:50:54, Josh Poimboeuf wrote:
>> On Mon, Jan 28, 2019 at 02:49:43PM -0500, Joe Lawrence wrote:
>> > On Sun, Jan 27, 2019 at 04:26:30AM +0900, Alice Ferrazzi wrote:
>> > > This patch fixes a checkpatch warning:
>> > > WARNING: ENOSYS means 'invalid syscall nr' and nothing else
>> > >
>> > > Signed-off-by: Alice Ferrazzi <alice.ferrazzi@miraclelinux.com>
>> > > ---
>> > > kernel/livepatch/core.c | 2 +-
>> > > 1 file changed, 1 insertion(+), 1 deletion(-)
>> > >
>> > > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
>> > > index 5b77a7314e01..eea6b94fef89 100644
>> > > --- a/kernel/livepatch/core.c
>> > > +++ b/kernel/livepatch/core.c
>> > > @@ -897,7 +897,7 @@ int klp_register_patch(struct klp_patch *patch)
>> > >
>> > > if (!klp_have_reliable_stack()) {
>> > > pr_err("This architecture doesn't have support for the livepatch consistency model.\n");
>> > > - return -ENOSYS;
>> > > + return -ENOTSUPP;
>> > > }
>> > >
>> > > return klp_init_patch(patch);
>> > > --
>> > > 2.19.2
>> > >
>> >
>> > Hi Alice,
>> >
>> > Patches should be based off the upstream livepatching tree, found here:
>> >
>> > git://git.kernel.org/pub/scm/linux/kernel/git/livepatching/livepatching.git
>> >
>> > and in this case, the for-next branch, which holds patches that have
>> > already been queued up for the next release. This one:
>> >
>> > 958ef1e39d24 ("livepatch: Simplify API by removing registration step")
>> >
>> > has moved the code in question from klp_register_patch() to
>> > klp_enable_patch().
>> >
>> >
>> > As far as the change itself, I don't have strong opinion about it
>> > either way.
>> >
>> > On the one hand, there is the checkpatch warning and -ENOTSUPP reads
>> > more intuitively than -ENOSYS.
>> >
>> > However, the current pattern seems to be more prevelent in the kernel.
>> > I wonder if the checkpatch warning would be better specified for return
>> > values that are actually passed back to userspace.
>> >
>> > Also, klp_register_patch(), now klp_enable_patch(), is exported for
>> > module use, though I don't believe anyone (samples / tests / kpatch /
>> > kgraft?) is inspecting which error value is returned.
>> >
>> > I would defer to whichever convention the maintainers prefer here.
>>
>> Based on the commit description from 91c9afaf97ee ("checkpatch.pl: new
>> instances of ENOSYS are errors"), it sounds like there was a decision
>> at
>> Kernel Summit to limit ENOSYS to mean "bad syscall" and nothing else.
>
> Hmm, the error code is passed to the syscall, for example:
>
> + SYSCALL_DEFINE3(init_module
> + load_module()
> + do_init_module()
> + do_one_initcall(mod->init);
>
> I am not sure if we are allowed to return -ENOTSUPP (-524).
> It is defined in the internal include/linux/errno.h. There
> is the following commnent:
>
> /*
> * These should never be seen by user programs...
>
>
>
> I tried to find a better alternative and found:
>
> #define EOPNOTSUPP 95 /* Operation not supported on transport endpoint
> */
>
>
> There is the following note in man errno:
>
> ENOTSUP Operation not supported (POSIX.1)
>
> EOPNOTSUPP Operation not supported on socket (POSIX.1)
> (ENOTSUP and EOPNOTSUPP have the same value
> on Linux, but according to POSIX.1 these error
> values should be distinct.)
>
> And it looks that -EOPNOTSUPP is used widely in many subsystes (not
> only network).
>
> Best Regards,
> Petr
EOPNOTSUPP works also for me.
looks better adopted than ENOTSUP.
I will send a new patch based off the upstream from
git://git.kernel.org/pub/scm/linux/kernel/git/livepatching/livepatching.git
as suggested by Joe
Thanks,
Alice
--
======================================
Alice Ferrazzi
alicef@alicef.me
PGP: 2E4E 0856 461C 0585 1336 F496 5621 A6B2 8638 781A
======================================
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] livepatch: core: Return ENOTSUPP instead of ENOSYS
2019-01-30 12:41 ` Petr Mladek
2019-01-30 13:00 ` alicef
@ 2019-01-30 13:10 ` Josh Poimboeuf
1 sibling, 0 replies; 6+ messages in thread
From: Josh Poimboeuf @ 2019-01-30 13:10 UTC (permalink / raw)
To: Petr Mladek
Cc: Joe Lawrence, Alice Ferrazzi, jeyu, jikos, mbenes, live-patching,
linux-kernel, Alice Ferrazzi
On Wed, Jan 30, 2019 at 01:41:56PM +0100, Petr Mladek wrote:
> Hmm, the error code is passed to the syscall, for example:
>
> + SYSCALL_DEFINE3(init_module
> + load_module()
> + do_init_module()
> + do_one_initcall(mod->init);
>
> I am not sure if we are allowed to return -ENOTSUPP (-524).
> It is defined in the internal include/linux/errno.h. There
> is the following commnent:
>
> /*
> * These should never be seen by user programs...
>
>
>
> I tried to find a better alternative and found:
>
> #define EOPNOTSUPP 95 /* Operation not supported on transport endpoint */
>
>
> There is the following note in man errno:
>
> ENOTSUP Operation not supported (POSIX.1)
>
> EOPNOTSUPP Operation not supported on socket (POSIX.1)
> (ENOTSUP and EOPNOTSUPP have the same value
> on Linux, but according to POSIX.1 these error
> values should be distinct.)
>
> And it looks that -EOPNOTSUPP is used widely in many subsystes (not
> only network).
Yes, you are right. It's confusing that ENOTSUPP and ENOTSUP are not
the same thing. EOPNOTSUPP sounds good.
--
Josh
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-01-30 13:10 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-26 19:26 [PATCH] livepatch: core: Return ENOTSUPP instead of ENOSYS Alice Ferrazzi
2019-01-28 19:49 ` Joe Lawrence
2019-01-29 16:50 ` Josh Poimboeuf
2019-01-30 12:41 ` Petr Mladek
2019-01-30 13:00 ` alicef
2019-01-30 13:10 ` Josh Poimboeuf
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).