On Tue, Sep 1, 2020 at 11:44 AM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> On Tue, Sep 1, 2020 at 12:17 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
> >
> > On Sun, Aug 30, 2020 at 10:28 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> > > + * The "work/incompat" directory is treated specially - if it is not
> > > + * empty, instead of printing a generic error and mounting read-only,
> > > + * we will error about incompat features and fail the mount.
> > > + */
> > > + if (level == 2 && !strcmp(path->dentry->d_name.name, OVL_INCOMPATDIR_NAME))
> > > + incompat = true;
> >
> > Should the test be specific to "work/incompat"? AFAICS this will
> > trigger under "index" as well...
>
> When called from ovl_indexdir_cleanup(), path->dentry->d_name.name[0] == '#',
> because cleanup starts at level 1 and ovl_workdir_cleanup_recurse() is called
> with level 2.
Okay. I'll add a comment.
> >
> > >
> > > err = ovl_dir_read(path, &rdd);
> > > if (err)
> > > @@ -1079,21 +1090,29 @@ static void ovl_workdir_cleanup_recurse(struct path *path, int level)
> > > continue;
> > > if (p->len == 2 && p->name[1] == '.')
> > > continue;
> > > + } else if (incompat) {
> > > + pr_warn("overlay with incompat feature '%.*s' cannot be mounted\n",
> > > + p->len, p->name);
> > > + err = -EEXIST;
> >
> > EEXIST feels counterintuitive. I'd rather opt for EINVAL.
>
> I usually prefer to use EINVAL for illegal user input and this is a border line,
> so I prefer errors that indicate the state of the object, like EEXIST
> or ENOTEMPTY,
> but because these errors are not expected on mount, I can live with EINVAL.
Exactly. The error should make sense in relation to a specific
argument of the syscall. The connection with EEXIST is too deeply
embedded inside overlayfs implementation details.
> Assuming that you agree with my response to ovl_indexdir_cleanup(), let
> me know if you need me to post v3 or if you can change the choice of error
> and s/pr_warn/pr_err on commit.
I'll fix these up.
Thanks,
Miklos
