Re: [PATCH] livepatch: core: Return ENOTSUPP instead of ENOSYS

[alicef <alicef@alicef.me>]

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
======================================