Re: [PATCH 3/7] ubifs: Add support for FS_IOC_FS[SG]ETXATTR ioctls

From: Richard Weinberger <richard.weinberger@gmail.com>
To: Sascha Hauer <s.hauer@pengutronix.de>
Cc: linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	Richard Weinberger <richard@nod.at>,
	linux-mtd@lists.infradead.org, kernel@pengutronix.de,
	Jan Kara <jack@suse.com>
Subject: Re: [PATCH 3/7] ubifs: Add support for FS_IOC_FS[SG]ETXATTR ioctls
Date: Sun, 19 Jan 2020 20:55:59 +0100	[thread overview]
Message-ID: <CAFLxGvxbcMF1S=Ghmi2rH4-ecEPRtVPAS7LrRq5eX=Q6S4PMHg@mail.gmail.com> (raw)
In-Reply-To: <20191106091537.32480-4-s.hauer@pengutronix.de>

On Wed, Nov 6, 2019 at 10:16 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> The FS_IOC_FS[SG]ETXATTR ioctls are an alternative to FS_IOC_[GS]ETFLAGS
> with additional features. This patch adds support for these ioctls.
>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  fs/ubifs/ioctl.c | 103 ++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 98 insertions(+), 5 deletions(-)
>
> diff --git a/fs/ubifs/ioctl.c b/fs/ubifs/ioctl.c
> index 8230dba5fd74..533df56beab4 100644
> --- a/fs/ubifs/ioctl.c
> +++ b/fs/ubifs/ioctl.c
> @@ -95,9 +95,46 @@ static int ubifs2ioctl(int ubifs_flags)
>         return ioctl_flags;
>  }
>
> -static int setflags(struct file *file, int flags)
> +/* Transfer xflags flags to internal */
> +static inline unsigned long ubifs_xflags_to_iflags(__u32 xflags)

Why inline? gcc should be smart enough to inline the function automatically
if needed.

>  {
> -       int oldflags, err, release;
> +       unsigned long iflags = 0;
> +
> +       if (xflags & FS_XFLAG_SYNC)
> +               iflags |= UBIFS_APPEND_FL;

Shouldn't this be UBIFS_SYNC_FL?

> +       if (xflags & FS_XFLAG_IMMUTABLE)
> +               iflags |= UBIFS_IMMUTABLE_FL;
> +       if (xflags & FS_XFLAG_APPEND)
> +               iflags |= UBIFS_APPEND_FL;
> +
> +        return iflags;
> +}
> +
> +/* Transfer internal flags to xflags */
> +static inline __u32 ubifs_iflags_to_xflags(unsigned long flags)

Same.

> +{
> +       __u32 xflags = 0;
> +
> +       if (flags & UBIFS_APPEND_FL)
> +               xflags |= FS_XFLAG_SYNC;
> +       if (flags & UBIFS_IMMUTABLE_FL)
> +               xflags |= FS_XFLAG_IMMUTABLE;
> +       if (flags & UBIFS_APPEND_FL)
> +               xflags |= FS_XFLAG_APPEND;
> +
> +        return xflags;
> +}
> +
> +static void ubifs_fill_fsxattr(struct inode *inode, struct fsxattr *fa)
> +{
> +       struct ubifs_inode *ui = ubifs_inode(inode);
> +
> +       simple_fill_fsxattr(fa, ubifs_iflags_to_xflags(ui->flags));
> +}
> +
> +static int setflags(struct file *file, int flags, struct fsxattr *fa)
> +{
> +       int ubi_flags, oldflags, err, release;
>         struct inode *inode = file_inode(file);
>         struct ubifs_inode *ui = ubifs_inode(inode);
>         struct ubifs_info *c = inode->i_sb->s_fs_info;
> @@ -110,6 +147,11 @@ static int setflags(struct file *file, int flags)
>         if (!inode_owner_or_capable(inode))
>                 return -EACCES;
>
> +       if (fa)
> +               ubi_flags = ubifs_xflags_to_iflags(fa->fsx_xflags);
> +       else
> +               ubi_flags = ioctl2ubifs(flags);
> +

So having both flags and fa set is not allowed?
Can we please have an ubifs_assert() to catch this.

-- 
Thanks,
//richard