On Wed, Jul 5, 2023 at 11:07 AM Alexander Larsson <alexl@xxxxxxxxxx> wrote:
>
> On Mon, Jul 3, 2023 at 9:14 PM Eric Biggers <ebiggers@xxxxxxxxxx> wrote:
> >
> > On Wed, Jun 21, 2023 at 01:18:26PM +0200, Alexander Larsson wrote:
> > > Historically overlay.metacopy was a zero-size xattr, and it's
> > > existence marked a metacopy file. This change adds a versioned header
> > > with a flag field, a length and a digest. The initial use-case of this
> > > will be for validating a fs-verity digest, but the flags field could
> > > also be used later for other new features.
> > >
> > > ovl_check_metacopy_xattr() now returns the size of the xattr,
> > > emulating a size of OVL_METACOPY_MIN_SIZE for empty xattrs to
> > > distinguish it from the no-xattr case.
> > >
> > > Signed-off-by: Alexander Larsson <alexl@xxxxxxxxxx>
> > > ---
> > > fs/overlayfs/namei.c | 10 +++++-----
> > > fs/overlayfs/overlayfs.h | 24 +++++++++++++++++++++++-
> > > fs/overlayfs/util.c | 37 +++++++++++++++++++++++++++++++++----
> > > 3 files changed, 61 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> > > index 57adf911735f..3dd480253710 100644
> > > --- a/fs/overlayfs/namei.c
> > > +++ b/fs/overlayfs/namei.c
> > > @@ -25,7 +25,7 @@ struct ovl_lookup_data {
> > > bool stop;
> > > bool last;
> > > char *redirect;
> > > - bool metacopy;
> > > + int metacopy;
> >
> > Should this be called 'metacopy_size' now?
>
> Honestly I don't know. That would change a lot of locations that still
> use this as "essentially" a boolean (i.e. != 0 means "has metacopy"),
> and ity would make that code less compact. I guess this is up to Amir
> and Miklos. Surely we could add a comment in the struct definition
> though.
>
I agree most of the code looks nicer when this stays 'metacopy'
> > > - err = ovl_check_metacopy_xattr(OVL_FS(d->sb), &path);
> > > + err = ovl_check_metacopy_xattr(OVL_FS(d->sb), &path, NULL);
> > > if (err < 0)
> > > goto out_err;
> >
> > This part is confusing because variables named 'err' conventionally contain only
> > 0 or a negative errno value. But this patch makes it possible for
> > ovl_check_metacopy_xattr() to return a positive size.
>
> It was already returning "negative, 0 or 1", so it's not fundamentally
> changed. Again, this is not my code so I'd rather Amir and Miklos
> decide such code style questions.
>
I agree. It wasn't 0 or negative before the change and I don't
think this "convention" justifies adding another var.
Thanks,
Amir,
