Re: [PATCH v5] printk: Userspace format enumeration support

[Chris Down]

Ack to all unmentioned suggestions. :-)

Petr Mladek writes:
> > +	  changed or no longer present.
> > +
> > +	  There is no additional runtime cost to printk with this enabled.
> > +
> >  #
> >  # Architectures with an unreliable sched_clock() should select this:
> >  #
> > diff --git a/kernel/module.c b/kernel/module.c
> > index 1e5aad812310..44df2913a046 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -1064,6 +1064,7 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
> >  	blocking_notifier_call_chain(&module_notify_list,
> >  				     MODULE_STATE_GOING, mod);
> >  	klp_module_going(mod);
> > +	pi_sec_remove(mod);
>
> Is there any particular reason why this is not done via the module
> notifier, please?
>
> Other subsystems hardcode their callbacks here only when they
> require some special ordering that could not be achieved by
> the notifiers.
>
> The hardcoded callbacks complicate the error paths in
> the module loader code.

Oh! That's exactly what I feel as well, but I mistakenly thought that's what 
you were asking for in the feedback for v4. Turns out I misread your statement 
about storing the pointer to `struct module` (hence my message last time 
querying whether it was sensible or not) as being about not using the module 
notifier. Mea culpa.

> > +static void *pi_next(struct seq_file *s, void *v, loff_t *pos)
> > +{
> > +	const struct pi_sec *ps = s->file->f_inode->i_private;
> > +	struct pi_object *pi = NULL;
>
> Please, call the variables by the content and not by prefix.
> A variable called "pi" might include anything used by "pi" API.
>
> [...]
>
> Please, try to put more effort into creating the function and
> variable names. I know that I am probably too picky about it.
> But you seem to be the other extreme.
>
> Inconsistent, ambiguous, or meaningless names might make even few
> lines of code hard to follow. It makes it write-only.
> It is hard to review and maintain.

Hmm, I'd even say that I agree with this statement, but as I understand it a 
`pi` variable always means pi_object, and `ps` always means pi_sec. I'm not 
immediately seeing it as meaningless or ambiguous (although maybe your concern 
was more abstractly aesthetic with overlapping the `pi_` prefix?).

The "content" here is pretty abstract, so I'm not quite sure what your 
suggestion for naming them based on content is. Maybe (assuming it doesn't just 
disappear, which it seems it will) a pi_sec named sec, and the pi_object named 
fmt_index? I don't feel strongly that this is more clear though, so maybe you 
mean something else?