On Mon, Oct 30, 2023 at 5:37 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote: > > 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? > oops. I guess I'll need to add a test case... > > 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. > What a mess. I prefer to restore the old pr_err messages with the pathname for now, because they are more likely to help the users fix the problem. We could sort it better when we add support for path parameters. At least with path params, we would know that it is not legacy mount API. Thanks, Amir.
