On Mon, 30 Oct 2023 at 13:04, Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > In preparation for new mount options to add lowerdirs one by one, > generalize ovl_parse_param_upperdir() into helper ovl_parse_layer() > with bool @upper argument that will be false for adding lower layers. > > Suggested-by: Miklos Szeredi <miklos@xxxxxxxxxx> > Link: https://lore.kernel.org/r/CAJfpegt7VC94KkRtb1dfHG8+4OzwPBLYqhtc8=QFUxpFJE+=RQ@xxxxxxxxxxxxxx/ > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx> > --- > fs/overlayfs/params.c | 116 ++++++++++++++++++++++-------------------- > 1 file changed, 62 insertions(+), 54 deletions(-) > > diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c > index 0bf754a69e91..9a9238eac730 100644 > --- a/fs/overlayfs/params.c > +++ b/fs/overlayfs/params.c > @@ -43,7 +43,7 @@ module_param_named(metacopy, ovl_metacopy_def, bool, 0644); > MODULE_PARM_DESC(metacopy, > "Default to on or off for the metadata only copy up feature"); > > -enum { > +enum ovl_opt { > Opt_lowerdir, > Opt_upperdir, > Opt_workdir, > @@ -238,19 +238,8 @@ static int ovl_mount_dir_noesc(const char *name, struct path *path) > pr_err("failed to resolve '%s': %i\n", name, err); > goto out; > } > - err = -EINVAL; > - if (ovl_dentry_weird(path->dentry)) { > - pr_err("filesystem on '%s' not supported\n", name); > - goto out_put; > - } > - if (!d_is_dir(path->dentry)) { > - pr_err("'%s' not a directory\n", name); > - goto out_put; > - } This will lose the check for lowerdir, no? > return 0; > > -out_put: > - path_put_init(path); > out: > return err; > } > @@ -268,7 +257,7 @@ static void ovl_unescape(char *s) > } > } > > -static int ovl_mount_dir(const char *name, struct path *path, bool upper) > +static int ovl_mount_dir(const char *name, struct path *path) > { > int err = -ENOMEM; > char *tmp = kstrdup(name, GFP_KERNEL); > @@ -276,60 +265,81 @@ static int ovl_mount_dir(const char *name, struct path *path, bool upper) > if (tmp) { > ovl_unescape(tmp); > err = ovl_mount_dir_noesc(tmp, path); > - > - if (!err && upper && path->dentry->d_flags & DCACHE_OP_REAL) { > - pr_err("filesystem on '%s' not supported as upperdir\n", > - tmp); > - path_put_init(path); > - err = -EINVAL; > - } > kfree(tmp); > } > return err; > } > > -static int ovl_parse_param_upperdir(const char *name, struct fs_context *fc, > - bool workdir) > +static int ovl_mount_dir_check(struct fs_context *fc, const struct path *path, > + enum ovl_opt layer, const char *name, bool upper) > { > - int err; > - struct ovl_fs *ofs = fc->s_fs_info; > - struct ovl_config *config = &ofs->config; > - struct ovl_fs_context *ctx = fc->fs_private; > - struct path path; > - char *dup; > + if (ovl_dentry_weird(path->dentry)) > + return invalfc(fc, "filesystem on %s not supported", name); > > - err = ovl_mount_dir(name, &path, true); > - if (err) > - return err; > + if (!d_is_dir(path->dentry)) > + return invalfc(fc, "%s is not a directory", name); This can result in: overlay: lowerdir+ is not a directory Which is somewhat confusing. Not sure how mount/libmount will present such option error messages, as that does not currently work. So the kernel could be really nice about it and tell the user which lowerdir (layer index). But libmount could also indicate which option failed, in which case indicating the layer would not be needed. OTOH when using the legacy API we do need to tell the user whether it was upperdir or workdir, but that doesn't affect lowerdir+. So some compromise and negotiation with util-linux devs is needed. Thanks, Miklos
