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

[Dave Chinner <david@fromorbit.com>]

On Fri, Oct 03, 2014 at 05:01:18PM -0500, Ben Myers wrote:
> From: Olaf Weber <olaf@sgi.com>
> 
> 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@sgi.com>
> 
> ---
> [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@fromorbit.com