On Thu, Mar 20, 2025 at 12:35:20PM -0400, Pasha Tatashin wrote: > On Thu, Mar 20, 2025 at 9:40 AM Andy Shevchenko > <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote: > > On Thu, Mar 20, 2025 at 02:40:09AM +0000, Pasha Tatashin wrote: ... > > > +#ifndef _LINUX_LIVEUPDATE_H > > > +#define _LINUX_LIVEUPDATE_H > > > > > +#include <linux/compiler.h> > > > +#include <linux/notifier.h> > > > > This is semi-random list of inclusions. Try to follow IWYU principle. > > See below. > > I will remove <linux/compiler.h> But you need to add something more... ... > > > +bool liveupdate_state_updated(void); > > > > Where bool is defined? > > in kernel/liveupdate.c Nope, I meant where the type is defined. It is IIRC in types.h which needs to be included. ... > > > + if (kstrtol(buf, 0, &val) < 0) > > > + return -EINVAL; > > > > Shadower error code. > > In this case it is appropriate, we do not case why kstrtol() could not > be converted into an appropriate integer, all we care is that the > input was invalid, and that what we return back to user. The kstrtox() may give different error codes. User may want to know more about what's wrong with the input. Shadowed error codes are discouraged and should be explained. > > > + if (val != 1 && val != 0) > > > + return -EINVAL; > > > > What's wrong with using kstrtobool() from the beginning? > > It makes the input less defined, here we only allow '1' or '0', > kstrtobool() allows almost anything. But kstrtobool() is the interface for boolean input. You may document only 0 and 1 and don't tell people to use anything else. ABI documentation should be clear, that's it. ... > > > +EXPORT_SYMBOL_GPL(liveupdate_state_normal); > > > > No namespace? > > Namespace is 'liveupdate_', all public interfaces have this prefix, > private functions are prefixed with luo_ where it makes sense. No, I'm talking about export namespace. Why does the entire kernel need these APIs? -- With Best Regards, Andy Shevchenko
