Re: [PATCH 18/41] regset: new method and helpers for it

[Al Viro <viro@zeniv.linux.org.uk>]

On Mon, Jun 29, 2020 at 12:23:34PM -0700, Linus Torvalds wrote:
> On Mon, Jun 29, 2020 at 11:28 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > ->get2() takes task+regset+buffer, returns the amount of free space
> > left in the buffer on success and -E... on error.
> 
> Can we please give it a better name than "get2"?
> 
> That's not a great name to begin with, and it's a completely
> nonsensical name by the end of this series when you've removed the
> original "get" function.
> 
> So either:
> 
>  (a) add one final patch to rename "get2" all back to "get" after you
> got rid of the old "get"

Bad idea, IMO...

>  (b) or just call it something better to begin with. Maybe just
> "get_regset" instead?

<nod>

Frankly, other field names there are also nasty - 'set' is not
fun to grep for, but there are 'n' and 'size' as well.  There's
also 'bias'  and 'align' (both completely unused)...

> I'd prefer (b) just because I think it will be a lot clearer if we
> ever end up having old patches forward-ported (or, more likely,
> newpatches back-ported), so having a "get" function that changed
> semantics but got back the old name sounds bad to me.
> 
> Other than that, I can't really argue with the numbers:
> 
>  41 files changed, 1368 insertions(+), 2347 deletions(-)
> 
> looks good to me, and the code seems more understandable.

FWIW, there's also ->set() side of that thing.  Tons of boilerplate
in those; I hadn't seriously looked into that part, but I suspect
that "do copyin in the caller, pass the kernel buffer to the
method, always start at offset 0" would also trim a lot of crap.
I'd rather leave that one for later - this series had been painful
enough.

Other things in the same area: conversion of fdpic coredumps to
regset (fairly easy, I've a patch series doing that in the local
tree), conversion of the few remaining non-regset architectures
to regset-based coredump (kill a large chunk of rotting code in
fs/binfmt_elf.c, along with quite a few pieces of old per-arch
coredump helpers), unification of compat ELF (done locally,
mipsn32/mipso32 gone, all compat done via compat_binfmt_elf.c,
x32 horrors sanitized - IMO
#define PRSTATUS_SIZE \
       (test_thread_flag(TIF_X32) \
               ? sizeof(struct compat_elf_prstatus) \
               : sizeof(struct i386_elf_prstatus))
#define SET_PR_FPVALID(S) \
       (*(test_thread_flag(TIF_X32) \
               ? &(S)->pr_fpvalid      \
               : &((struct i386_elf_prstatus *)(S))->pr_fpvalid) = 1)
is much better than
#define PRSTATUS_SIZE(S, R) (R != sizeof(S.pr_reg) ? 144 : 296)
#define SET_PR_FPVALID(S, V, R) \
  do { *(int *) (((void *) &((S)->pr_reg)) + R) = (V); } \
  while (0)
) and a bunch of assorted cleanups that got trimmed from the
regset series...

I'll probably post fdpic and compat series tonight and get
back to uaccess and VFS stuff.