Messages in this thread | | | Date | Wed, 30 Jan 2019 22:00:51 +0900 | From | alicef <> | 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@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 ======================================
| |