On Jun 14, 2022, at 1:40 PM, John Hubbard <jhubbard@xxxxxxxxxx> wrote:
> On 6/14/22 11:56, Mike Rapoport wrote:
>>> But, I cannot take it anymore: the list of arguments for uffd stuff is
>>> crazy. I would like to collect all the possible arguments that are used for
>>> uffd operation into some “struct uffd_op”.
>> Squashing boolean parameters into int flags will also reduce the insane
>> amount of parameters. No strong feelings though.
>>
>
> Just a quick drive-by comment about boolean arguments: they ruin the
> readability of the call sites. In practice, sometimes a single boolean
> argument can be OK-ish (still poor to read at the call site, but easier
> to code initially), but once you get past one boolean argument in the
> function, readability is hopeless:
>
> foo(ptr, true, false, a == b);
>
> So if you have a choice, I implore you to prefer flags and/or enums. :)
Thanks for the feedback - I am aware it is very confusing to have booleans
and especially multiple ones in a func call.
Just not sure how it maps to what I proposed. I thought of passing as an
argument reference (pointer) to something similar to the following struct,
which I think is very self-descriptive:
struct uffd_op {
/* various fields */
struct vm_area_struct *dst_vma;
unsigned long len;
atomic_t *mmap_changing;
...
/* ... and some flags */
int wp: 1;
int zero: 1;
int read_likely: 1;
...
};
I think that fits what you were asking for. The only thing I am not sure of,
is whether to include in uffd_op fields that are internal to mm/userfaultfd
such as “page” and “newly_allocated”. I guess not.
