On Tue, Oct 10, 2023 at 7:13 PM Sebastian Wick
<sebastian.wick@xxxxxxxxxx> wrote:
>
> On Tue, Oct 10, 2023 at 12:00 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> >
> > On Tue, Oct 10, 2023 at 12:06 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
> > >
> > > On Fri, 6 Oct 2023 at 19:21, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> > > >
> > > > On Fri, Oct 6, 2023 at 7:42 PM Ryan Hendrickson
> > >
> > > > > And there is the escaping that needs to happen for ':' and '\' when
> > > > > parsing the path parameters (':' is only special syntax in lowerdir, but
> > > > > the escaping logic seems to apply to upperdir and workdir as well, based
> > > > > on my testing). Even using the new API, this is handled in the kernel.
> > > > > We'd like to know if this escaping can be considered stable as well, and I
> > > > > don't think that's a question for the libmount maintainer.
> > > >
> > > > Agree.
> > > > Unlike the comma separated parameters list,
> > > > upperdir,workdir,lowerdir are overlayfs specific format.
> > > >
> > > > ovl_unescape() (for upperdir/workdir) unescapes '\' characters.
> > > > as does ovl_parse_param_split_lowerdirs().
> > > > Not sure why this was needed for upperdir/workdir, but it It has
> > > > been this way for a long time.
> > > > I see no reason for it to change in the future.
> > >
> > > Unescaping upperdir/workdir was the side effect of using a common
> > > helper; it wasn't intentional, I think. The problem is that
> > > unescaping breaks code that doesn't expect it, and filenames with
> > > backslashes (and especially \\ or \: sequences) are very rare, so this
> > > won't show up in testing.
> > >
> > > At this point I'm not sure which is more likely to cause bugs: getting
> > > rid of unescaping or leaving it alone.
> >
> > Considering the fact that the applications that mount overlayfs has
> > always had to do the correct escaping, getting rid of escaping can
> > only solve issues in new deployments, so I think we should greatly
> > favor leaving it alone.
>
> Any change here is a regression. I'm seriously confused why this is
> even debated. You already managed to have a regression and I'm still
> of the opinion that this should be fixed because it literally breaks
> user space.
>
You are right. Literally it does.
But if prospect users are ok with upgrading libmount and if that
solves the problem, I'd rather not have to carry in the kernel
baggage code to support old mount API for a very niche use case.
> > >
> > > One way out of this mess is to create explicit _unesc versions of these options.
> > >
> >
> > I like that solution, with two reservations:
> > 1. IMO, new _unesc versions should only be supported from new mount API
> > 2. I only want to do that if real users exists - said users are expected
> > to send the patch and explain their use case
>
> This is confusing me a lot. Why would you not want to provide an API
> which is clearly, objectively the better API? As user space, when we
> can use the new mount API and we could use this, we absolutely would
> use this.
I am also confused by this reaction.
Who said that I do not want to provide the _unenc API?
IIUC, you are requesting a new feature that did not exist before,
namely, upperdir_unenc, workdir_unenc, lowerdir_unenc options.
Did I understand correctly?
If that is the case then please send a patch to support
those new options in the new mount API only
including documentation and tests.
Thanks,
Amir.
