So from a quick scan through the patches, they all looked really nice
(but note the "quick scan" - I didn't apply this, much less test
anything).
I did react to this one, though - not very complicated, but this patch
struck me as kind of ugly compared to most of the others.
On Sun, 22 Jun 2025 at 21:54, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>
> - gemfs = vfs_kern_mount(type, SB_KERNMOUNT, type->name, huge_opt);
> - if (IS_ERR(gemfs))
> + fc = fs_context_for_mount(type, SB_KERNMOUNT);
> + if (IS_ERR(fc))
> + goto err;
> + ret = add_param(fc, "source", "tmpfs");
> + if (!ret)
> + ret = add_param(fc, "huge", "within_size");
> + if (!ret)
> + gemfs = fc_mount_longterm(fc);
> + put_fs_context(fc);
> + if (ret)
> goto err;
So this "fs_context_for_mount() + N * add_param()" pattern ends up
showing up twice, with that 'add_param()' helper done twice too.
And that's ignoring the _existing_ users of "fs_context_for_mount() +
N * vfs_parse_fs_string()", which are really the same except they
don't wrap it with that 'add_param()' helper.
I'm not objecting to the patch, and I don't really even have a
solution: many of the existing cases actually do need the more
complicated vfs_parse_fs_string() interface because they don't want
that simple 'strlen()' for size.
I just feel that at a minimum you shouldn't implement add_param()
twice, because some other users *would* want to do that.
So I wish you had made that a real helper - which would obviously then
also force a naming change ("fs_context_add_param()".
Or maybe even go further and some helper to doi that
"fs_context_for_mount()" _with_ a list of param's to be added?
I do think that could be done later (separately), but wanted to just
mention this because I reacted to this patch.
Linus
