[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <0ae0746120590dc3c5f7c1d602321f28@alicef.me>
Date: Wed, 30 Jan 2019 22:00:51 +0900
From: alicef <alicef@...cef.me>
To: Petr Mladek <pmladek@...e.com>
Cc: Josh Poimboeuf <jpoimboe@...hat.com>,
Joe Lawrence <joe.lawrence@...hat.com>, jeyu@...nel.org,
jikos@...nel.org, mbenes@...e.cz, live-patching@...r.kernel.org,
linux-kernel@...r.kernel.org,
Alice Ferrazzi <alice.ferrazzi@...aclelinux.com>,
live-patching-owner@...r.kernel.org
Subject: Re: [PATCH] livepatch: core: Return ENOTSUPP instead of ENOSYS
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@...aclelinux.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@...cef.me
PGP: 2E4E 0856 461C 0585 1336 F496 5621 A6B2 8638 781A
======================================
Powered by blists - more mailing lists