Re: [PATCH v2 14/31] selftests/mm: uffd_[un]register()

[Peter Xu]

On Wed, Apr 12, 2023 at 11:20:35AM -0700, Axel Rasmussen wrote:
> > -       uffdio_register.range.start = (unsigned long)addr;
> > -       uffdio_register.range.len = len;
> > -       uffdio_register.mode = UFFDIO_REGISTER_MODE_MISSING;
> > -       if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register) == -1) {
> > +       if (uffd_register(uffd, addr, len, true, false, false)) {
> 
> For what it's worth, I agree with Mike that the booleans here are hard
> to read. It's not clear what "true, false, false" means without going
> to look at the header file, whereas "uffd_register(uffd, addr, len,
> MINOR)" would be immediately clear.
> 
> One solution I've seen outside the kernel is to comment the bools, like:
> 
> uffd_register(..., /*minor=*/true, /*wp=*/false, /*minor=*/false);
> 
> But, then I feel we lose most of the benefit we wanted from switching
> to bools anyway (code length). :)
> 
> I do agree the macro names are unwieldy, and lots of tests already use
> booleans so we'd have to convert from bool -> flag. If it were me, I
> would resolve that by:
> 
> - Define much shorter aliases for these macros in uffd-common.h
> - Consider refactoring callers to not use bools either.
> 
> Then again, I also agree with Mike that it's not a deal breaker, if
> you like this way much more than that alternative we can leave it.

No strong feeling here either.  I kept it just to avoid code churns and
rebases.

I'll see whether I'll need a repost, if so I can go back to use modes.

Thanks,

-- 
Peter Xu