xfs
[Top] [All Lists]

Re: [PATCH 11/16] xfs: add xfs_nameops for utf8 and utf8+casefold.

To: Ben Myers <bpm@xxxxxxx>
Subject: Re: [PATCH 11/16] xfs: add xfs_nameops for utf8 and utf8+casefold.
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Tue, 7 Oct 2014 09:10:16 +1100
Cc: linux-fsdevel@xxxxxxxxxxxxxxx, olaf@xxxxxxx, xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20141003220118.GJ1865@xxxxxxx>
References: <20141003214758.GY1865@xxxxxxx> <20141003220118.GJ1865@xxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Fri, Oct 03, 2014 at 05:01:18PM -0500, Ben Myers wrote:
> From: Olaf Weber <olaf@xxxxxxx>
> 
> The xfs_utf8_nameops use the nfkdi normalization when comparing filenames,
> and are installed if the utf8bit is set in the super block.
> 
> The xfs_utf8_ci_nameops use the nfkdicf normalization when comparing
> filenames, and are installed if both the utf8bit and the borgbit are set
> in the superblock.
> 
> Normalized filenames are not stored on disk. Normalization will fail if a
> filename is not valid UTF-8, in which case the filename is treated as an
> opaque blob.
> 
> Signed-off-by: Olaf Weber <olaf@xxxxxxx>
> 
> ---
> [v2: updated to use utf8norm.ko module;
>      compiled conditionally on CONFIG_XFS_UTF8=y;
>      utf8version is now a function;
>      move xfs_utf8.[ch] into libxfs. --bpm]
> [v3: pass utf8version from the superblock through xfs_nameops
>      instead of the max version of the normalization module. --bpm]
> ---
>  fs/xfs/Kconfig           |   9 ++
>  fs/xfs/Makefile          |   2 +
>  fs/xfs/libxfs/xfs_dir2.c |   4 +-
>  fs/xfs/libxfs/xfs_utf8.c | 208 
> +++++++++++++++++++++++++++++++++++++++++++++++
>  fs/xfs/libxfs/xfs_utf8.h |   3 +
>  fs/xfs/xfs_iops.c        |   2 +-
>  6 files changed, 225 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/Kconfig b/fs/xfs/Kconfig
> index 5d47b4d..1e8a463 100644
> --- a/fs/xfs/Kconfig
> +++ b/fs/xfs/Kconfig
> @@ -95,3 +95,12 @@ config XFS_DEBUG
>         not useful unless you are debugging a particular problem.
>  
>         Say N unless you are an XFS developer, or you play one on TV.
> +
> +config XFS_UTF8
> +     bool "XFS UTF-8 support"
> +     depends on XFS_FS
> +     select CONFIG_UTF8_NORMALIZATION
> +     help
> +       Say Y here to enable utf8 normalization support in XFS.  You
> +       will be able to mount and use filesystems created with the
> +       utf8 mkfs.xfs option.

"created with UTF8 support enabled."

> diff --git a/fs/xfs/Makefile b/fs/xfs/Makefile
> index d617999..192aaca 100644
> --- a/fs/xfs/Makefile
> +++ b/fs/xfs/Makefile
> @@ -114,6 +114,8 @@ xfs-$(CONFIG_XFS_QUOTA)           += xfs_dquot.o \
>                                  xfs_qm.o \
>                                  xfs_quotaops.o
>  
> +xfs-$(CONFIG_XFS_UTF8)               += libxfs/xfs_utf8.o
> +

libxfs definitions come first. Also, please use the same prefixing
syntax that the other libxfs rules use.

>  # xfs_rtbitmap is shared with libxfs
>  xfs-$(CONFIG_XFS_RT)         += xfs_rtalloc.o
>  
> diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c
> index 2c89211..9cfbd6b 100644
> --- a/fs/xfs/libxfs/xfs_dir2.c
> +++ b/fs/xfs/libxfs/xfs_dir2.c
> @@ -165,9 +165,9 @@ xfs_da_mount(
>               /* XXX these are replaced in the next patch need
>                  to do some kind of reordering here */
>               if (xfs_sb_version_hasasciici(&mp->m_sb))
> -                     mp->m_dirnameops = &xfs_ascii_ci_nameops;
> +                     mp->m_dirnameops = &xfs_utf8_ci_nameops;
>               else
> -                     mp->m_dirnameops = &xfs_default_nameops;
> +                     mp->m_dirnameops = &xfs_utf8_nameops;
>  #else

xfs_sb_version_hasasciici()? The overloading of the asciici bit is
still used for the utf8 CI functionality? Please fix this for the
next version of the patchset.

>               xfs_warn(mp,
>  "Recompile XFS with CONFIG_XFS_UTF8 to mount this filesystem");
> diff --git a/fs/xfs/libxfs/xfs_utf8.c b/fs/xfs/libxfs/xfs_utf8.c
> index 7e63111..1e75299 100644
> --- a/fs/xfs/libxfs/xfs_utf8.c
> +++ b/fs/xfs/libxfs/xfs_utf8.c
> @@ -68,3 +68,211 @@ xfs_utf8_version_ok(
>  
>       return 0;
>  }
> +
> +/*
> + * xfs nameops using nfkdi
> + */

Remind me again what nfkdi means? I I can't remember the details
after a week or two, then perhaps better explanitory comments are
needed in the code?

> +static xfs_dahash_t
> +xfs_utf8_hashname(
> +     const unsigned char *name,
> +     int len,
> +     unsigned int sb_utf8version)

Please use the same indentation levels for the declartions. i.e

        const unsigned char     *name,
        int                     len,
        unsigned int            sb_utf8version)

Can you go through all the XFS code and make sure this is done?

> +{
> +     utf8data_t      nfkdi;
> +     struct utf8cursor u8c;
> +     xfs_dahash_t    hash;
> +     int             val;

And these shold line up, too.

> +
> +     nfkdi = utf8nfkdi(sb_utf8version);
> +     hash = 0;

initialise at declaration.

> +     if (utf8ncursor(&u8c, nfkdi, name, len) < 0)
> +             goto blob;

Still has the "invalid binary blob" issue.

> +     while ((val = utf8byte(&u8c)) > 0)
> +             hash = val ^ rol32(hash, 7);
> +     /* In case of error treat the name as a binary blob. */
> +     if (val == 0)
> +             return hash;
> +blob:
> +     return xfs_da_hashname(name, len);
> +}
> +
> +static int
> +xfs_utf8_normhash(

More commments needed explaining what is going on.

> +     struct xfs_da_args *args)
> +{
> +     utf8data_t      nfkdi;
> +     struct utf8cursor u8c;
> +     unsigned char   *norm;
> +     ssize_t         normlen;
> +     int             c;
> +     unsigned int    sb_utf8version =
> +             args->dp->i_mount->m_sb.sb_utf8version;

Urk. Initialise on a separate line.

> +
> +     nfkdi = utf8nfkdi(sb_utf8version);
> +     /* Failure to normalize is treated as a blob. */
> +     if ((normlen = utf8nlen(nfkdi, args->name, args->namelen)) < 0)
> +             goto blob;

No assignments in logic statements, please.

        normlen = utf8nlen(nfkdi, args->name, args->namelen);
        if (normlen < 0)

This is all through the code - can you please go through and fix up
all the patches to remove this pattern? checkpatch might be helpful
here....

As it is, still has the invalid binary blob issue.


> +     if (utf8ncursor(&u8c, nfkdi, args->name, args->namelen) < 0)
> +             goto blob;
> +     if (!(norm = kmem_alloc(normlen + 1, KM_NOFS|KM_MAYFAIL)))
> +             return -ENOMEM;

Urk.

So, what happens if this memory allocation fails in the middle of a
create transaction?

(Hint: transaction is dirty at this point in time)

The rest of the code in this patch has similar issues to what I've
already pointed out.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>