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

[alicef]

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@xxxxxxxxxxxxxxxx>
> > > > ---
> > > >  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@xxxxxxxxx
PGP: 2E4E 0856 461C 0585 1336 F496 5621 A6B2 8638 781A
======================================