From: Amir Goldstein <amir73il@gmail.com>
To: Alexander Larsson <alexl@redhat.com>
Cc: Eric Biggers <ebiggers@kernel.org>,
miklos@szeredi.hu, linux-unionfs@vger.kernel.org, tytso@mit.edu,
fsverity@lists.linux.dev
Subject: Re: [PATCH v4 2/4] ovl: Add versioned header for overlay.metacopy xattr
Date: Wed, 5 Jul 2023 16:12:06 +0300 [thread overview]
Message-ID: <CAOQ4uxgfNrMEKyOxGV3L2sUauM47-L77PhwNYooY43kFF+fycw@mail.gmail.com> (raw)
In-Reply-To: <CAL7ro1GpGczvGN28yVNMOw_9Uz-2SEJcRUMmvoBdmEWO5ynb7g@mail.gmail.com>
On Wed, Jul 5, 2023 at 11:07 AM Alexander Larsson <alexl@redhat.com> wrote:
>
> On Mon, Jul 3, 2023 at 9:14 PM Eric Biggers <ebiggers@kernel.org> 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@redhat.com>
> > > ---
> > > 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,
next prev parent reply other threads:[~2023-07-05 13:12 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-21 11:18 [PATCH v5 0/4] ovl: Add support for fs-verity checking of lowerdata Alexander Larsson
2023-06-21 11:18 ` [PATCH v4 1/4] ovl: Add framework for verity support Alexander Larsson
2023-06-21 12:18 ` Amir Goldstein
2023-07-03 19:08 ` Eric Biggers
2023-06-21 11:18 ` [PATCH v4 2/4] ovl: Add versioned header for overlay.metacopy xattr Alexander Larsson
2023-06-21 12:21 ` Amir Goldstein
2023-07-03 19:13 ` Eric Biggers
2023-07-05 8:07 ` Alexander Larsson
2023-07-05 13:12 ` Amir Goldstein [this message]
2023-06-21 11:18 ` [PATCH v4 3/4] ovl: Validate verity xattr when resolving lowerdata Alexander Larsson
2023-06-21 12:24 ` Amir Goldstein
2023-07-03 19:24 ` Eric Biggers
2023-07-05 9:09 ` Alexander Larsson
2023-06-21 11:18 ` [PATCH v4 4/4] ovl: Handle verity during copy-up Alexander Larsson
2023-06-21 12:26 ` Amir Goldstein
2023-07-03 19:29 ` Eric Biggers
2023-07-05 9:11 ` Alexander Larsson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAOQ4uxgfNrMEKyOxGV3L2sUauM47-L77PhwNYooY43kFF+fycw@mail.gmail.com \
--to=amir73il@gmail.com \
--cc=alexl@redhat.com \
--cc=ebiggers@kernel.org \
--cc=fsverity@lists.linux.dev \
--cc=linux-unionfs@vger.kernel.org \
--cc=miklos@szeredi.hu \
--cc=tytso@mit.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).