xfs
[Top] [All Lists]

Re: [REVIEW 1/2] Case insensitive support for XFS - kernel patch

To: Barry Naujok <bnaujok@xxxxxxx>
Subject: Re: [REVIEW 1/2] Case insensitive support for XFS - kernel patch
From: Eric Sandeen <sandeen@xxxxxxxxxxx>
Date: Fri, 18 Jan 2008 23:22:47 -0600
Cc: "xfs@xxxxxxxxxxx" <xfs@xxxxxxxxxxx>, xfs-dev <xfs-dev@xxxxxxx>
In-reply-to: <op.t43zfc073jf8g2@xxxxxxxxxxxxxxxxxxxxxxxxxxxx>
References: <op.t43zfc073jf8g2@xxxxxxxxxxxxxxxxxxxxxxxxxxxx>
Sender: xfs-bounce@xxxxxxxxxxx
User-agent: Thunderbird 2.0.0.9 (Macintosh/20071031)
Barry Naujok wrote:
> This patch should apply to 2.6.24-rc6.

actually doesn't seem to; at least doesn't apply to rc8... mount
handling has moved, etc - well, easy enough to fix up.

The samba guys are excited about this, I bet :)

Lots of comments below; keep an open mind and a sense of humor, most
aren't demands but ideas, musings etc...


> ===========================================================================
> fs/xfs/Makefile
> ===========================================================================
> 
> --- a/fs/xfs/Makefile 2008-01-18 15:31:23.000000000 +1100
> +++ b/fs/xfs/Makefile 2007-10-23 16:17:22.173903950 +1000
> @@ -74,6 +74,7 @@ xfs-y                               += xfs_alloc.o \
>                                  xfs_trans_extfree.o \
>                                  xfs_trans_inode.o \
>                                  xfs_trans_item.o \
> +                                xfs_unicode.o \

obj-$(CONFIG_XFS_UNICODE)  perhaps?  It's a lot of new code that not
everyone needs?

>                                  xfs_utils.o \
>                                  xfs_vfsops.o \
>                                  xfs_vnodeops.o \
> 
...

> ===========================================================================
> fs/xfs/linux-2.6/xfs_iops.c
> ===========================================================================
> 
> --- a/fs/xfs/linux-2.6/xfs_iops.c     2008-01-18 15:31:23.000000000 +1100
> +++ b/fs/xfs/linux-2.6/xfs_iops.c     2008-01-17 12:26:26.905427167 +1100
> @@ -47,6 +47,8 @@
>  #include "xfs_buf_item.h"
>  #include "xfs_utils.h"
>  #include "xfs_vnodeops.h"
> +#include "xfs_da_btree.h"
> +#include "xfs_unicode.h"
>  
>  #include <linux/capability.h>
>  #include <linux/xattr.h>
> @@ -388,7 +390,7 @@ xfs_vn_lookup(
>       if (dentry->d_name.len >= MAXNAMELEN)
>               return ERR_PTR(-ENAMETOOLONG);
>  
> -     error = xfs_lookup(XFS_I(dir), dentry, &cvp);
> +     error = xfs_lookup(XFS_I(dir), dentry, &cvp, NULL, NULL);
>       if (unlikely(error)) {
>               if (unlikely(error != ENOENT))
>                       return ERR_PTR(-error);
> @@ -399,6 +401,113 @@ xfs_vn_lookup(
>       return d_splice_alias(vn_to_inode(cvp), dentry);
>  }
>  
> +STATIC struct dentry *
> +xfs_vn_ci_lookup(
> +     struct inode    *dir,
> +     struct dentry   *dentry,
> +     struct nameidata *nd)
> +{
> +     bhv_vnode_t     *cvp;
> +     int             error;
> +     struct dentry   *result;
> +     struct qstr     actual_name;
> +     struct inode    *inode;
> +
> +     if (dentry->d_name.len >= MAXNAMELEN)
> +             return ERR_PTR(-ENAMETOOLONG);
> +
> +     error = xfs_lookup(XFS_I(dir), dentry, &cvp, (char **)&actual_name.name,
> +                     &actual_name.len);
> +     if (unlikely(error)) {
> +             if (unlikely(error != ENOENT))
> +                     return ERR_PTR(-error);
> +             d_add(dentry, NULL);
> +             return NULL;
> +     }
> +     inode = vn_to_inode(cvp);
> +
> +     /* if exact match, just splice and exit */
> +     if (!actual_name.name) {
> +             result = d_splice_alias(inode, dentry);
> +             return result;
> +     }
> +
> +     /*
> +      * case-insensitive match, create a dentry to return and fill it
> +      * in with the correctly cased name. Parameter "dentry" is not
> +      * used anymore and the caller will free it.
> +      * Derived from fs/ntfs/namei.c
> +      */
> +
> +     actual_name.hash = full_name_hash(actual_name.name, actual_name.len);
> +
> +     /* Does an existing dentry match? */
> +     result = d_lookup(dentry->d_parent, &actual_name);
> +     if (!result) {
> +             /* if not, create one */
> +             result = d_alloc(dentry->d_parent, &actual_name);
> +             xfs_free_unicode_nls_name((char *)actual_name.name);
> +             if (!result)
> +                     return ERR_PTR(-ENOMEM);
> +             dentry = d_splice_alias(inode, result);
> +             if (dentry) {
> +                     dput(result);
> +                     return dentry;
> +             }
> +             return result;
> +     }
> +     xfs_free_unicode_nls_name((char *)actual_name.name);
> +
> +     /* an existing dentry matches, use it */
> +
> +     if (result->d_inode) {
> +             /*
> +              * already an inode attached, deref the inode that was
> +              * refcounted with xfs_lookup and return the dentry.
> +              */
> +             if (unlikely(result->d_inode != inode)) {
> +                     /* This can happen because bad inodes are unhashed. */
> +                     BUG_ON(!is_bad_inode(inode));
> +                     BUG_ON(!is_bad_inode(result->d_inode));
> +             }
> +             iput(inode);
> +             return result;
> +     }
> +
> +     if (!S_ISDIR(inode->i_mode)) {
> +             /* not a directory, easy to handle */
> +             d_instantiate(result, inode);
> +             return result;
> +     }
> +
> +     spin_lock(&dcache_lock);
> +     if (list_empty(&inode->i_dentry)) {
> +             /*
> +              * Directory without a 'disconnected' dentry; we need to do
> +              * d_instantiate() by hand because it takes dcache_lock which
> +              * we already hold.
> +              */
> +             list_add(&result->d_alias, &inode->i_dentry);
> +             result->d_inode = inode;
> +             spin_unlock(&dcache_lock);
> +             security_d_instantiate(result, inode);
> +             return result;
> +     }
> +     /*
> +      * Directory with a 'disconnected' dentry; get a reference to the
> +      * 'disconnected' dentry.
> +      */
> +     dentry = list_entry(inode->i_dentry.next, struct dentry, d_alias);
> +     dget_locked(dentry);
> +     spin_unlock(&dcache_lock);
> +     security_d_instantiate(result, inode);
> +     d_move(dentry, result);
> +     iput(inode);
> +     dput(result);
> +     return dentry;
> +}
> +
> +

It seems like it might be nice to make the CI code a compile-time
option?  Fairly big new chunks... if it could be done cleanly, might be
nice...

> ===========================================================================
> fs/xfs/linux-2.6/xfs_linux.h
> ===========================================================================
> 
> --- a/fs/xfs/linux-2.6/xfs_linux.h    2008-01-18 15:31:24.000000000 +1100
> +++ b/fs/xfs/linux-2.6/xfs_linux.h    2008-01-11 14:49:16.537591564 +1100
> @@ -75,6 +75,8 @@
>  #include <linux/delay.h>
>  #include <linux/log2.h>
>  #include <linux/spinlock.h>
> +#include <linux/ctype.h>
> +#include <linux/nls.h>
>  
>  #include <asm/page.h>
>  #include <asm/div64.h>
> @@ -180,6 +182,12 @@
>  #define howmany(x, y)        (((x)+((y)-1))/(y))
>  
>  /*
> + * NLS UTF-8 character set
> + */
> +
> +#define XFS_NLS_UTF8 "utf8"

I guess that had to go somewhere? :)

> +
> +/*
>   * Various platform dependent calls that don't fit anywhere else
>   */
>  #define xfs_sort(a,n,s,fn)   sort(a,n,s,fn,NULL)
> 
> ===========================================================================
> fs/xfs/linux-2.6/xfs_super.c
> ===========================================================================
> 
> --- a/fs/xfs/linux-2.6/xfs_super.c    2008-01-18 15:31:24.000000000 +1100
> +++ b/fs/xfs/linux-2.6/xfs_super.c    2008-01-11 14:46:25.067566854 +1100
> @@ -50,6 +50,7 @@
>  #include "xfs_vnodeops.h"
>  #include "xfs_vfsops.h"
>  #include "xfs_version.h"
> +#include "xfs_unicode.h"
>  #include "xfs_log_priv.h"
>  
>  #include <linux/namei.h>
> @@ -121,6 +122,9 @@ xfs_args_allocate(
>  #define MNTOPT_ATTR2 "attr2"         /* do use attr2 attribute format */
>  #define MNTOPT_NOATTR2       "noattr2"       /* do not use attr2 attribute 
> format */
>  #define MNTOPT_FILESTREAM  "filestreams" /* use filestreams allocator */
> +#define MNTOPT_NLS   "nls"           /* NLS code page to use */
> +#define MNTOPT_CILOOKUP      "ci"            /* case-insensitive dir names */
> +#define MNTOPT_CIATTR        "ciattr"        /* case-insensitive attr names 
> */
>  #define MNTOPT_QUOTA "quota"         /* disk quotas (user) */
>  #define MNTOPT_NOQUOTA       "noquota"       /* no quotas */
>  #define MNTOPT_USRQUOTA      "usrquota"      /* user quota enabled */


Please document in Documentation/filesystems/xfs.txt too...

> @@ -315,6 +319,18 @@ xfs_parseargs(
>                       args->flags &= ~XFSMNT_ATTR2;
>               } else if (!strcmp(this_char, MNTOPT_FILESTREAM)) {
>                       args->flags2 |= XFSMNT2_FILESTREAMS;
> +             } else if (!strcmp(this_char, MNTOPT_NLS)) {
> +                     if (!value || !*value) {
> +                             cmn_err(CE_WARN,
> +                                     "XFS: %s option requires an argument",
> +                                     this_char);
> +                             return EINVAL;
> +                     }
> +                     strncpy(args->nls, value, MAXNAMELEN);
> +             } else if (!strcmp(this_char, MNTOPT_CILOOKUP)) {
> +                     args->flags2 |= XFSMNT2_CILOOKUP;
> +             } else if (!strcmp(this_char, MNTOPT_CIATTR)) {
> +                     args->flags2 |= XFSMNT2_CIATTR;
>               } else if (!strcmp(this_char, MNTOPT_NOQUOTA)) {
>                       args->flags &= ~(XFSMNT_UQUOTAENF|XFSMNT_UQUOTA);
>                       args->flags &= ~(XFSMNT_GQUOTAENF|XFSMNT_GQUOTA);
> @@ -454,6 +470,8 @@ xfs_showargs(
>               { XFS_MOUNT_OSYNCISOSYNC,       "," MNTOPT_OSYNCISOSYNC },
>               { XFS_MOUNT_ATTR2,              "," MNTOPT_ATTR2 },
>               { XFS_MOUNT_FILESTREAMS,        "," MNTOPT_FILESTREAM },
> +             { XFS_MOUNT_CI_LOOKUP,          "," MNTOPT_CILOOKUP },
> +             { XFS_MOUNT_CI_ATTR,            "," MNTOPT_CIATTR },
>               { XFS_MOUNT_DMAPI,              "," MNTOPT_DMAPI },
>               { XFS_MOUNT_GRPID,              "," MNTOPT_GRPID },
>               { 0, NULL }
> @@ -516,6 +534,13 @@ xfs_showargs(
>       if (!(mp->m_qflags & XFS_ALL_QUOTA_ACCT))
>               seq_puts(m, "," MNTOPT_NOQUOTA);
>  
> +     if (xfs_sb_version_hasunicode(&mp->m_sb)) {
> +             if (mp->m_nls)
> +                     seq_printf(m, "," MNTOPT_NLS "=%s", mp->m_nls->charset);
> +             else
> +                     seq_puts(m, "," MNTOPT_NLS "=" XFS_NLS_UTF8);
> +     }
> +
>       return 0;
>  }
>  __uint64_t
> @@ -563,7 +588,11 @@ xfs_set_inodeops(
>               inode->i_mapping->a_ops = &xfs_address_space_operations;
>               break;
>       case S_IFDIR:
> -             inode->i_op = &xfs_dir_inode_operations;
> +             inode->i_op =
> +                     xfs_sb_version_hasoldci(&XFS_I(inode)->i_mount->m_sb) ||
> +                     (XFS_I(inode)->i_mount->m_flags & XFS_MOUNT_CI_LOOKUP) ?
> +                             &xfs_dir_ci_inode_operations :
> +                             &xfs_dir_inode_operations;

Do any linux filesystems in existence actually have
XFS_SB_VERSION_OLDCIBIT?  I'd think not - this patch is *adding* that
macro - what's this for?

>               inode->i_fop = &xfs_dir_file_operations;
>               break;
>       case S_IFLNK:
> 
> ===========================================================================
> fs/xfs/xfs_attr.c
> ===========================================================================
> 
> --- a/fs/xfs/xfs_attr.c       2008-01-18 15:31:24.000000000 +1100
> +++ b/fs/xfs/xfs_attr.c       2008-01-18 13:25:20.068339942 +1100
> @@ -106,6 +106,17 @@ ktrace_t *xfs_attr_trace_buf;
>   * Overall external interface routines.
>   *========================================================================*/
>  
> +void
> +xfs_attr_mount(struct xfs_mount *mp)
> +{
> +     mp->m_attr_magicpct = (mp->m_sb.sb_blocksize * 37) / 100;
> +     if (xfs_sb_version_hasunicode(&mp->m_sb)) {
> +             mp->m_attrnameops = (mp->m_flags & XFS_MOUNT_CI_ATTR) ?
> +                     &xfs_unicode_ci_nameops : &xfs_unicode_nameops;
> +     } else
> +             mp->m_attrnameops = &xfs_default_nameops;
> +}

Hm, I thought most little mount-helper subroutines went right in next to
xfs_mountfs() in xfs_mount.c; just a thought.  Then you wouldn't need
the prototype in the header either...

> +
>  int
>  xfs_attr_fetch(xfs_inode_t *ip, const char *name, int namelen,
>              char *value, int *valuelenp, int flags, struct cred *cred)
> @@ -122,14 +133,14 @@ xfs_attr_fetch(xfs_inode_t *ip, const ch
>        * Fill in the arg structure for this request.
>        */
>       memset((char *)&args, 0, sizeof(args));
> -     args.name = name;
> -     args.namelen = namelen;
>       args.value = value;
>       args.valuelen = *valuelenp;
>       args.flags = flags;
> -     args.hashval = xfs_da_hashname(args.name, args.namelen);
>       args.dp = ip;
>       args.whichfork = XFS_ATTR_FORK;
> +     error = xfs_da_setup_name_and_hash(&args, name, namelen);
> +     if (error)
> +             return error;

In the spirit of incremental-but-functional patches, which helps to
break down & review big patchsets like this, I think this addition of
xfs_da_setup_name_and_hash() could stand on its own, and add the nls
stuff in a subsequent patch?

>       /*
>        * Decide on what work routines to call based on the inode size.
> @@ -153,6 +164,7 @@ xfs_attr_fetch(xfs_inode_t *ip, const ch
>  
>       if (error == EEXIST)
>               error = 0;
> +     xfs_da_cleanup_name(&args, name);

I'm being a little lazy here; under which circumstances would args->name
!= name, and need to be freed?

...

> ===========================================================================
> fs/xfs/xfs_attr_leaf.c
> ===========================================================================
> 
> --- a/fs/xfs/xfs_attr_leaf.c  2008-01-18 15:31:24.000000000 +1100
> +++ b/fs/xfs/xfs_attr_leaf.c  2008-01-18 13:25:11.873394723 +1100
> @@ -42,6 +42,7 @@
>  #include "xfs_attr.h"
>  #include "xfs_attr_leaf.h"
>  #include "xfs_error.h"
> +#include "xfs_unicode.h"
>  
>  /*
>   * xfs_attr_leaf.c
> @@ -90,6 +91,10 @@ STATIC void xfs_attr_leaf_moveents(xfs_a
>                                        xfs_mount_t *mp);
>  STATIC int xfs_attr_leaf_entsize(xfs_attr_leafblock_t *leaf, int index);
>  
> +STATIC int xfs_attr_put_listent(xfs_attr_list_context_t *context,
> +                                     attrnames_t *namesp, char *name,
> +                                     int namelen, int valuelen, char *value);
> +
>  /*========================================================================
>   * Namespace helper routines
>   *========================================================================*/
> @@ -135,6 +140,38 @@ xfs_attr_namesp_match_overrides(int arg_
>   * External routines when attribute fork size < XFS_LITINO(mp).
>   *========================================================================*/
>  
> +STATIC xfs_attr_sf_entry_t *
> +xfs_attr_shortform_find_ent(xfs_da_args_t *args)
> +{
> +     xfs_attr_shortform_t *sf;
> +     xfs_attr_sf_entry_t *sfe;
> +     int i;
> +     xfs_attr_sf_entry_t *ci_sfe = NULL;
> +
> +     ASSERT(args->dp->i_afp->if_flags & XFS_IFINLINE);
> +     sf = (xfs_attr_shortform_t *)args->dp->i_afp->if_u1.if_data;
> +     sfe = &sf->list[0];
> +
> +     args->cmpresult = XFS_CMP_DIFFERENT;
> +     for (i = 0; i < sf->hdr.count; sfe = XFS_ATTR_SF_NEXTENTRY(sfe), i++) {
> +             if (!xfs_attr_namesp_match(args->flags, sfe->flags))
> +                     continue;
> +             switch (xfs_attr_compname(args->dp, sfe->nameval, sfe->namelen,
> +                             args->name, args->namelen)) {
> +             case XFS_CMP_EXACT:
> +                     args->cmpresult = XFS_CMP_EXACT;
> +                     return sfe;
> +             case XFS_CMP_CASE:
> +                     if (!ci_sfe) {
> +                             args->cmpresult = XFS_CMP_CASE;
> +                             ci_sfe = sfe;
> +                     }
> +             default:;
> +             }
> +     }
> +     return ci_sfe;
> +}

Perhaps this helper could be a sub-patch too?  Just a thought.

>  /*
>   * Query whether the requested number of additional bytes of extended
>   * attribute space will be able to fit inline.
> @@ -295,13 +332,10 @@ xfs_attr_shortform_add(xfs_da_args_t *ar
>       sfe = &sf->list[0];
>       for (i = 0; i < sf->hdr.count; sfe = XFS_ATTR_SF_NEXTENTRY(sfe), i++) {
>  #ifdef DEBUG
> -             if (sfe->namelen != args->namelen)
> -                     continue;
> -             if (memcmp(args->name, sfe->nameval, args->namelen) != 0)
> -                     continue;
>               if (!xfs_attr_namesp_match(args->flags, sfe->flags))
>                       continue;
> -             ASSERT(0);
> +             ASSERT(xfs_attr_compname(args->dp, args->name, args->namelen,
> +                     sfe->nameval, sfe->namelen) == XFS_CMP_DIFFERENT);

Perhaps this helper too could come as a subpatch.

...


> +     if (!sfe)
> +             return XFS_ERROR(ENOATTR);
> +

Indentation from here on looks busted.  Are you missing braces?

>               if (args->flags & ATTR_KERNOVAL) {
>                       args->valuelen = sfe->valuelen;
> -                     return(XFS_ERROR(EEXIST));
> +             return XFS_ERROR(EEXIST);

need another tab here?

>               }
>               if (args->valuelen < sfe->valuelen) {
>                       args->valuelen = sfe->valuelen;
> -                     return(XFS_ERROR(ERANGE));
> +             return XFS_ERROR(ERANGE);

and here?

>               }
>               args->valuelen = sfe->valuelen;
> -             memcpy(args->value, &sfe->nameval[args->namelen],
> -                                                 args->valuelen);
> -             return(XFS_ERROR(EEXIST));
> -     }
> -     return(XFS_ERROR(ENOATTR));
> +     memcpy(args->value, &sfe->nameval[args->namelen], args->valuelen);
> +
> +     return XFS_ERROR(EEXIST);
>  }
>  
>  /*
...


> @@ -631,7 +626,7 @@ xfs_attr_shortform_list(xfs_attr_list_co
>                               continue;
>                       }
>                       namesp = xfs_attr_flags_namesp(sfe->flags);
> -                     error = context->put_listent(context,
> +                     error = xfs_attr_put_listent(context,
>                                          namesp,
>                                          (char *)sfe->nameval,
>                                          (int)sfe->namelen,
> @@ -734,7 +729,7 @@ xfs_attr_shortform_list(xfs_attr_list_co
>                       cursor->hashval = sbp->hash;
>                       cursor->offset = 0;
>               }
> -             error = context->put_listent(context,
> +             error = xfs_attr_put_listent(context,
>                                       namesp,
>                                       sbp->name,
>                                       sbp->namelen,

maybe the introduction of xfs_attr_put_listent could be a small
prep-work patch too.  Or is this getting old ;)

> @@ -1960,6 +1955,7 @@ xfs_attr_leaf_lookup_int(xfs_dabuf_t *bp
>       xfs_attr_leaf_name_remote_t *name_rmt;
>       int probe, span;
>       xfs_dahash_t hashval;
> +     xfs_dacmp_t cmp;
>  
>       leaf = bp->data;
>       ASSERT(be16_to_cpu(leaf->hdr.info.magic) == XFS_ATTR_LEAF_MAGIC);
> @@ -2008,6 +2004,7 @@ xfs_attr_leaf_lookup_int(xfs_dabuf_t *bp
>       /*
>        * Duplicate keys may be present, so search all of them for a match.
>        */
> +     args->cmpresult = XFS_CMP_DIFFERENT;
>       for (  ; (probe < be16_to_cpu(leaf->hdr.count)) &&
>                       (be32_to_cpu(entry->hashval) == hashval);
>                       entry++, probe++) {
> @@ -2019,35 +2016,40 @@ xfs_attr_leaf_lookup_int(xfs_dabuf_t *bp
>                * If we are looking for complete entries, show only those.
>                */
>               if ((args->flags & XFS_ATTR_INCOMPLETE) !=
> -                 (entry->flags & XFS_ATTR_INCOMPLETE)) {
> -                     continue;
> -             }
> -             if (entry->flags & XFS_ATTR_LOCAL) {
> -                     name_loc = XFS_ATTR_LEAF_NAME_LOCAL(leaf, probe);
> -                     if (name_loc->namelen != args->namelen)
> -                             continue;
> -                     if (memcmp(args->name, (char *)name_loc->nameval, 
> args->namelen) != 0)
> +                             (entry->flags & XFS_ATTR_INCOMPLETE))
>                               continue;
>                       if (!xfs_attr_namesp_match(args->flags, entry->flags))
>                               continue;
> +             if (entry->flags & XFS_ATTR_LOCAL) {
> +                     name_loc = XFS_ATTR_LEAF_NAME_LOCAL(leaf, probe);
> +                     cmp = xfs_attr_compname(args->dp, args->name, 
> args->namelen,
> +                                     name_loc->nameval, name_loc->namelen);
> +                     if (cmp != XFS_CMP_DIFFERENT && cmp != args->cmpresult) 
> {
> +                             args->cmpresult = cmp;
>                       args->index = probe;

weird indentation here too...

> -                     return(XFS_ERROR(EEXIST));
> +                             args->rmtblkno = 0;
> +                             args->rmtblkcnt = 0;
> +                             if (cmp == XFS_CMP_EXACT)
> +                                     return XFS_ERROR(EEXIST);
> +                     }
>               } else {
>                       name_rmt = XFS_ATTR_LEAF_NAME_REMOTE(leaf, probe);
> -                     if (name_rmt->namelen != args->namelen)
> -                             continue;
> -                     if (memcmp(args->name, (char *)name_rmt->name,
> -                                          args->namelen) != 0)
> -                             continue;
> -                     if (!xfs_attr_namesp_match(args->flags, entry->flags))
> -                             continue;
> +                     cmp = xfs_attr_compname(args->dp, args->name, 
> args->namelen,
> +                                     name_rmt->name, name_rmt->namelen);
> +                     if (cmp != XFS_CMP_DIFFERENT && cmp != args->cmpresult) 
> {
> +                             args->cmpresult = cmp;
>                       args->index = probe;
>                       args->rmtblkno = be32_to_cpu(name_rmt->valueblk);
>                       args->rmtblkcnt = XFS_B_TO_FSB(args->dp->i_mount,
>                                                  
> be32_to_cpu(name_rmt->valuelen));
> -                     return(XFS_ERROR(EEXIST));
> +                             if (cmp == XFS_CMP_EXACT)
> +                                     return XFS_ERROR(EEXIST);

and here

> +                     }
>               }
>       }
> +     if (args->cmpresult == XFS_CMP_CASE)
> +             return XFS_ERROR(EEXIST);
> +
>       args->index = probe;
>       return(XFS_ERROR(ENOATTR));
>  }
> @@ -2418,7 +2420,7 @@ xfs_attr_leaf_list_int(xfs_dabuf_t *bp, 
>                       xfs_attr_leaf_name_local_t *name_loc =
>                               XFS_ATTR_LEAF_NAME_LOCAL(leaf, i);
>  
> -                     retval = context->put_listent(context,
> +                     retval = xfs_attr_put_listent(context,

sub-patch? :)

...                             (int)name_rmt->namelen,
> @@ -2472,6 +2474,31 @@ xfs_attr_leaf_list_int(xfs_dabuf_t *bp, 
>       return(retval);
>  }
>  
> +/*
> + * Do NLS name conversion if required for attribute name and call
> + * context's put_listent routine
> + */
> +
> +STATIC int
> +xfs_attr_put_listent(xfs_attr_list_context_t *context, attrnames_t *namesp,
> +     char *name, int namelen, int valuelen, char *value)
> +{
> +     xfs_mount_t *mp = context->dp->i_mount;
> +     char *nls_name = NULL;
> +     int rval;
> +
> +     if (!mp->m_nls)
> +             return context->put_listent(context, namesp, name, namelen,
> +                             valuelen, value);
> +
> +     rval = xfs_unicode_to_nls(mp->m_nls, name, namelen, &nls_name);
> +     if (rval < 0)
> +             return -rval;
> +     rval = context->put_listent(context, namesp, nls_name, rval,
> +                     valuelen, value);
> +     xfs_free_unicode_nls_name(nls_name);
> +     return rval;
> +}
>  
>  /*========================================================================
>   * Manage the INCOMPLETE flag in a leaf entry
> ===========================================================================
> fs/xfs/xfs_attr_leaf.h
> ===========================================================================
> 
> --- a/fs/xfs/xfs_attr_leaf.h  2008-01-18 15:31:24.000000000 +1100
> +++ b/fs/xfs/xfs_attr_leaf.h  2008-01-11 14:16:44.268796245 +1100
> @@ -36,6 +36,7 @@ struct xfs_da_args;
>  struct xfs_da_state;
>  struct xfs_da_state_blk;
>  struct xfs_inode;
> +struct xfs_mount;
>  struct xfs_trans;
>  
>  /*========================================================================
> @@ -204,6 +205,16 @@ static inline int xfs_attr_leaf_entsize_
>       return (((bsize) >> 1) + ((bsize) >> 2));
>  }
>  
> +/*
> + * Do hash and name compare based on nameops
> + */
> +#define xfs_attr_hashname(dp, n, l) \
> +             ((dp)->i_mount->m_attrnameops->hashname((dp), (n), (l)))
> +
> +#define xfs_attr_compname(dp, n1, l1, n2, l2) \
> +             ((dp)->i_mount->m_attrnameops->compname((dp), (n1), (l1), \
> +                                                     (n2), (l2)))
> +
>  
>  /*========================================================================
>   * Structure used to pass context around among the routines.
> @@ -296,6 +307,7 @@ int       xfs_attr_root_inactive(struct xfs_tr
>  /*
>   * Utility routines.
>   */
> +void xfs_attr_mount(struct xfs_mount *mp);

this could just go in xfs_mount.c and be static.

>  xfs_dahash_t xfs_attr_leaf_lasthash(struct xfs_dabuf *bp, int *count);
>  int  xfs_attr_leaf_order(struct xfs_dabuf *leaf1_bp,
>                                  struct xfs_dabuf *leaf2_bp);
> 

> ===========================================================================
> fs/xfs/xfs_da_btree.c
> ===========================================================================
> 
> --- a/fs/xfs/xfs_da_btree.c   2008-01-18 15:31:24.000000000 +1100
> +++ b/fs/xfs/xfs_da_btree.c   2007-10-31 16:04:16.463309546 +1100
> @@ -46,6 +46,7 @@
>  #include "xfs_dir2_block.h"
>  #include "xfs_dir2_node.h"
>  #include "xfs_error.h"
> +#include "xfs_unicode.h"
>  
>  /*
>   * xfs_da_btree.c
> @@ -1530,6 +1531,100 @@ xfs_da_hashname(const uchar_t *name, int
>       }
>  }
>  
> +
> +static xfs_dahash_t
> +xfs_default_hashname(xfs_inode_t *inode, const uchar_t *name, int namelen)
> +{
> +     return xfs_da_hashname(name, namelen);
> +}

Could these be rolled into one instead of the wrapper?  I see 3 other
direct callers of xfs_da_hashname... and does xfs_attr_shortform_list
need to use xfs_attr_hashname instead of xfs_da_hashname?  Do "." and
".." ever have different answers in different codepages?  Does that matter?

> +xfs_dacmp_t
> +xfs_default_compname(xfs_inode_t *inode, const uchar_t *name1, int len1,
> +     const uchar_t *name2, int len2)
> +{
> +     return (len1 == len2 && memcmp(name1, name2, len1) == 0) ?
> +                     XFS_CMP_EXACT : XFS_CMP_DIFFERENT;
> +}
> +
> +static xfs_dahash_t
> +xfs_unicode_ci_hashname(
> +     xfs_inode_t     *inode,
> +     const uchar_t   *name,
> +     int             namelen)
> +{
> +     return xfs_unicode_hash(inode->i_mount->m_cft, name, namelen);
> +}

this wrapper is the only caller of xfs_unicode_hash?  Is it just to
conveniently get from inode to m_cft?

strikes me as a little interesting that while an inode is passed into a
xfs_hashname_t, it's never actually used directly - same for compname -
would it make any sense to just pass the xfs_cft in from the start?

...

> ===========================================================================
> fs/xfs/xfs_dir2.c
> ===========================================================================
> 
> --- a/fs/xfs/xfs_dir2.c       2008-01-18 15:31:24.000000000 +1100
> +++ b/fs/xfs/xfs_dir2.c       2008-01-11 14:24:51.701973714 +1100
> @@ -42,8 +42,56 @@
>  #include "xfs_dir2_node.h"
>  #include "xfs_dir2_trace.h"
>  #include "xfs_error.h"
> +#include "xfs_unicode.h"
>  #include "xfs_vnodeops.h"
>  
> +/*
> + * V1 case-insensitive support for directories
> + */

remind me, what's "V1?"

> +static xfs_dahash_t
> +xfs_ascii_ci_hashname(
> +     xfs_inode_t     *inode,
> +     const uchar_t   *name,
> +     int             namelen)
> +{
> +     xfs_dahash_t    hash;
> +     int             i;
> +
> +     for (i = 0, hash = 0; i < namelen; i++)
> +             hash = tolower(name[i]) ^ rol32(hash, 7);
> +
> +     return hash;
> +}
> +
> +static xfs_dacmp_t
> +xfs_ascii_ci_compname(
> +     xfs_inode_t     *inode,
> +     const uchar_t   *name1,
> +     int             len1,
> +     const uchar_t   *name2,
> +     int             len2)
> +{
> +     xfs_dacmp_t     result = XFS_CMP_EXACT;
> +     int             i;
> +
> +     if (len1 != len2)
> +             return XFS_CMP_DIFFERENT;
> +
> +     for (i = 0; i < len1; i++) {
> +             if (name1[i] == name2[i])
> +                     continue;
> +             if (tolower(name1[i]) != tolower(name2[i]))
> +                     return XFS_CMP_DIFFERENT;
> +             result = XFS_CMP_CASE;
> +     }
> +
> +     return result;
> +}
> +
> +static const struct xfs_nameops xfs_ascii_ci_nameops = {
> +     .hashname       = xfs_ascii_ci_hashname,
> +     .compname       = xfs_ascii_ci_compname,
> +};
>  
>  void
>  xfs_dir_mount(
> @@ -64,6 +112,13 @@ xfs_dir_mount(
>               (mp->m_dirblksize - (uint)sizeof(xfs_da_node_hdr_t)) /
>               (uint)sizeof(xfs_da_node_entry_t);
>       mp->m_dir_magicpct = (mp->m_dirblksize * 37) / 100;
> +
> +     if (xfs_sb_version_hasunicode(&mp->m_sb)) {
> +             mp->m_dirnameops = (mp->m_flags & XFS_MOUNT_CI_LOOKUP) ?
> +                     &xfs_unicode_ci_nameops : &xfs_unicode_nameops;
> +     } else
> +             mp->m_dirnameops = (xfs_sb_version_hasoldci(&mp->m_sb)) ?
> +                     &xfs_ascii_ci_nameops : &xfs_default_nameops;

Oh, "V1" is oldci - can you document it that way in the comment?  And if
that hasn't been seen in the wild on linux can this go away?

...

> ===========================================================================
> fs/xfs/xfs_dir2_block.c
> ===========================================================================
> 
> --- a/fs/xfs/xfs_dir2_block.c 2008-01-18 15:31:24.000000000 +1100
> +++ b/fs/xfs/xfs_dir2_block.c 2008-01-11 14:28:44.763934272 +1100
...

> @@ -697,20 +720,34 @@ xfs_dir2_block_lookup_int(
>               dep = (xfs_dir2_data_entry_t *)
>                       ((char *)block + xfs_dir2_dataptr_to_off(mp, addr));
>               /*
> -              * Compare, if it's right give back buffer & entry number.
> -              */
> -             if (dep->namelen == args->namelen &&
> -                 dep->name[0] == args->name[0] &&
> -                 memcmp(dep->name, args->name, args->namelen) == 0) {
> +              * Compare, if it's right give back buffer & entry number:
> +              *
> +              * lookup case - use nameops;
> +              *
> +              * replace/remove case - as lookup has been already been
> +              * performed, look for an exact match using the fast method
> +              */
> +             cmp = args->oknoent ?
> +                     xfs_dir_compname(dp, dep->name, dep->namelen,
> +                                             args->name, args->namelen) :
> +                     xfs_default_compname(dp, dep->name, dep->namelen,
> +                                             args->name, args->namelen);
> +             if (cmp != XFS_CMP_DIFFERENT && cmp != args->cmpresult) {
> +                     args->cmpresult = cmp;
>                       *bpp = bp;
>                       *entno = mid;
> +                     if (cmp == XFS_CMP_EXACT)
>                       return 0;
>               }
> -     } while (++mid < be32_to_cpu(btp->count) && 
> be32_to_cpu(blp[mid].hashval) == hash);
> +     } while (++mid < be32_to_cpu(btp->count) &&
> +                     be32_to_cpu(blp[mid].hashval) == hash);

thanks... can you trim down the other > 80 char lines you've added, too? :)

> @@ -1300,10 +1313,17 @@ xfs_dir2_leaf_lookup(
>       /*
>        * Return the found inode number.
>        */
> +     error = EEXIST;
>       args->inumber = be64_to_cpu(dep->inumber);
> +     if (args->cmpresult == XFS_CMP_CASE) {
> +             args->valuelen = xfs_unicode_to_nls(args->dp->i_mount->m_nls,
> +                             dep->name, dep->namelen, (char **)&args->value);
> +             if (args->valuelen < 0)
> +                     error = -args->valuelen;

hm, error signs... new functions return negative, whereas old xfs code
returns positive errors?

...

> @@ -1391,19 +1413,31 @@ xfs_dir2_leaf_lookup_int(
>                      xfs_dir2_dataptr_to_off(mp, be32_to_cpu(lep->address)));
>               /*
>                * If it matches then return it.
> -              */
> -             if (dep->namelen == args->namelen &&
> -                 dep->name[0] == args->name[0] &&
> -                 memcmp(dep->name, args->name, args->namelen) == 0) {
> +              *
> +              * lookup case - use nameops;
> +              *
> +              * replace/remove case - as lookup has been already been
> +              * performed, look for an exact match using the fast method
> +              */
> +             cmp = args->oknoent ?
> +                     xfs_dir_compname(dp, dep->name, dep->namelen,
> +                                             args->name, args->namelen) :
> +                     xfs_default_compname(dp, dep->name, dep->namelen,
> +                                             args->name, args->namelen);
> +             if (cmp != XFS_CMP_DIFFERENT && cmp != args->cmpresult) {
> +                     args->cmpresult = cmp;
>                       *dbpp = dbp;
>                       *indexp = index;
> +                     if (cmp == XFS_CMP_EXACT)
>                       return 0;

tab missing?


> ===========================================================================
> fs/xfs/xfs_dir2_node.c
> ===========================================================================
> 
> --- a/fs/xfs/xfs_dir2_node.c  2008-01-18 15:31:25.000000000 +1100
> +++ b/fs/xfs/xfs_dir2_node.c  2007-10-31 12:32:04.060201390 +1100
...

> @@ -572,28 +575,34 @@ xfs_dir2_leafn_lookup_int(
>                       /*
>                        * Point to the data entry.
>                        */
> -                     dep = (xfs_dir2_data_entry_t *)
> -                           ((char *)curbp->data +
> +                     dep = (xfs_dir2_data_entry_t *)((char *)curbp->data +
>                              xfs_dir2_dataptr_to_off(mp, 
> be32_to_cpu(lep->address)));
>                       /*
>                        * Compare the entry, return it if it matches.
>                        */
> -                     if (dep->namelen == args->namelen &&
> -                         dep->name[0] == args->name[0] &&
> -                         memcmp(dep->name, args->name, args->namelen) == 0) {
> +                     cmp = args->oknoent ?
> +                             xfs_dir_compname(dp, dep->name, dep->namelen,
> +                                             args->name, args->namelen):
> +                             xfs_default_compname(dp, dep->name, 
> dep->namelen,
> +                                             args->name, args->namelen);
> +                     if (cmp != XFS_CMP_DIFFERENT &&
> +                                     cmp != args->cmpresult) {
> +                             args->cmpresult = cmp;
>                               args->inumber = be64_to_cpu(dep->inumber);
>                               *indexp = index;
>                               state->extravalid = 1;
>                               state->extrablk.bp = curbp;
>                               state->extrablk.blkno = curdb;
> -                             state->extrablk.index =
> -                                     (int)((char *)dep -
> +                             state->extrablk.index = (int)((char *)dep -
>                                             (char *)curbp->data);
>                               state->extrablk.magic = XFS_DIR2_DATA_MAGIC;
> +                             if (cmp == XFS_CMP_EXACT)
>                               return XFS_ERROR(EEXIST);

tab...  Hmm wonder if I caught all of these...

> ===========================================================================
> fs/xfs/xfs_dir2_sf.c
> ===========================================================================
> 
> --- a/fs/xfs/xfs_dir2_sf.c    2008-01-18 15:31:25.000000000 +1100
> +++ b/fs/xfs/xfs_dir2_sf.c    2008-01-17 12:25:01.552398622 +1100
> @@ -38,6 +38,7 @@
>  #include "xfs_dir2_leaf.h"
>  #include "xfs_dir2_block.h"
>  #include "xfs_dir2_trace.h"
> +#include "xfs_unicode.h"
>  
>  /*
>   * Prototypes for internal functions.
> @@ -708,6 +709,8 @@ xfs_dir2_sf_getdents(
>       xfs_dir2_dataptr_t      dot_offset;
>       xfs_dir2_dataptr_t      dotdot_offset;
>       xfs_ino_t               ino;
> +     char                    *nls_name = NULL; /* NLS name buffer */
> +     int                     nls_namelen = 0;
>  
>       mp = dp->i_mount;
>  
> @@ -772,6 +775,9 @@ xfs_dir2_sf_getdents(
>               }
>       }
>  
> +     if (mp->m_nls)
> +             nls_name = xfs_alloc_unicode_nls_name();
> +
>       /*
>        * Loop while there are more entries and put'ing works.
>        */
> @@ -789,16 +795,22 @@ xfs_dir2_sf_getdents(
>  #if XFS_BIG_INUMS
>               ino += mp->m_inoadd;
>  #endif
> -
> -             if (filldir(dirent, sfep->name, sfep->namelen,
> +             if (mp->m_nls)
> +                     nls_namelen = xfs_unicode_to_nls(mp->m_nls, sfep->name,
> +                                     sfep->namelen, &nls_name);
> +             if (filldir(dirent,
> +                             nls_namelen > 0 ? nls_name : (char *)sfep->name,
> +                             nls_namelen > 0 ? nls_namelen : sfep->namelen,
>                                           off, ino, DT_UNKNOWN)) {

Hmm we've seen the foo > 0 ? bar : baz stuff a few times, should this
get a helper?

...

> @@ -844,23 +857,43 @@ xfs_dir2_sf_lookup(
>       if (args->namelen == 2 &&
>           args->name[0] == '.' && args->name[1] == '.') {
>               args->inumber = xfs_dir2_sf_get_inumber(sfp, &sfp->hdr.parent);
> +             args->cmpresult = XFS_CMP_EXACT;
>               return XFS_ERROR(EEXIST);
>       }
>       /*
>        * Loop over all the entries trying to match ours.
>        */
> -     for (i = 0, sfep = xfs_dir2_sf_firstentry(sfp);
> -          i < sfp->hdr.count;
> +     args->cmpresult = XFS_CMP_DIFFERENT;
> +     for (i = 0, sfep = xfs_dir2_sf_firstentry(sfp); i < sfp->hdr.count;
>            i++, sfep = xfs_dir2_sf_nextentry(sfp, sfep)) {
> -             if (sfep->namelen == args->namelen &&
> -                 sfep->name[0] == args->name[0] &&
> -                 memcmp(args->name, sfep->name, args->namelen) == 0) {
> -                     args->inumber =
> -                             xfs_dir2_sf_get_inumber(sfp,
> +             switch (xfs_dir_compname(dp, sfep->name, sfep->namelen,
> +                             args->name, args->namelen)) {
> +             case XFS_CMP_EXACT:
> +                     args->cmpresult = XFS_CMP_EXACT;
> +                     args->inumber = xfs_dir2_sf_get_inumber(sfp,
>                                       xfs_dir2_sf_inumberp(sfep));
> +                     if (args->value) {
> +                             xfs_free_unicode_nls_name(args->value);
> +                             args->value = NULL;
> +                     }
>                       return XFS_ERROR(EEXIST);
> +
> +             case XFS_CMP_CASE:
> +                     if (!args->value) {
> +                             args->valuelen = xfs_unicode_to_nls(
> +                                     args->dp->i_mount->m_nls, sfep->name,
> +                                     sfep->namelen, (char **)&args->value);
> +                             if (args->valuelen < 0)
> +                                     return XFS_ERROR(-args->valuelen);
> +                             args->cmpresult = XFS_CMP_CASE;
> +                             args->inumber = xfs_dir2_sf_get_inumber(sfp,
> +                                             xfs_dir2_sf_inumberp(sfep));
> +                     }
> +             default: ;

Hmm that's a little funky, to my eyes anyway.

...

> ===========================================================================
> fs/xfs/xfs_mount.c
> ===========================================================================
> 
> --- a/fs/xfs/xfs_mount.c      2008-01-18 15:31:25.000000000 +1100
> +++ b/fs/xfs/xfs_mount.c      2008-01-17 17:10:29.777728874 +1100
> @@ -25,6 +25,7 @@
>  #include "xfs_sb.h"
>  #include "xfs_ag.h"
>  #include "xfs_dir2.h"
> +#include "xfs_attr.h"
>  #include "xfs_dmapi.h"
>  #include "xfs_mount.h"
>  #include "xfs_bmap_btree.h"
> @@ -43,6 +44,9 @@
>  #include "xfs_rw.h"
>  #include "xfs_quota.h"
>  #include "xfs_fsops.h"
> +#include "xfs_da_btree.h"
> +#include "xfs_attr_leaf.h"
> +#include "xfs_unicode.h"
>  
>  STATIC void  xfs_mount_log_sbunit(xfs_mount_t *, __int64_t);
>  STATIC int   xfs_uuid_mount(xfs_mount_t *);
> @@ -119,6 +123,8 @@ static const struct {
>      { offsetof(xfs_sb_t, sb_logsectsize),0 },
>      { offsetof(xfs_sb_t, sb_logsunit),        0 },
>      { offsetof(xfs_sb_t, sb_features2),       0 },
> +    { offsetof(xfs_sb_t, sb_bad_features2), 0 },

bad_features2?  what's this and what does it have to do w/ CI, and why
is it set but never used in xfs_sb_from_disk?

if features2 "could be here" shouldn't we be doing something with that?
 This could do with comments at least, somewhere.

...

> @@ -1165,6 +1176,17 @@ xfs_mountfs(
>       }
>  
>       /*
> +      * Load in unicode case folding table from disk
> +      */
> +     if (xfs_sb_version_hasunicode(&mp->m_sb)) {
> +             error = xfs_unicode_read_cft(mp);
> +             if (error) {
> +                     cmn_err(CE_WARN, "XFS: failed to read case folding 
> table");

80+ chars...

..

> ===========================================================================
> fs/xfs/xfs_sb.h
> ===========================================================================
> 
> --- a/fs/xfs/xfs_sb.h 2008-01-18 15:31:25.000000000 +1100
> +++ b/fs/xfs/xfs_sb.h 2007-10-23 16:55:47.440178601 +1000
> @@ -46,10 +46,12 @@ struct xfs_mount;
>  #define XFS_SB_VERSION_SECTORBIT     0x0800
>  #define      XFS_SB_VERSION_EXTFLGBIT        0x1000
>  #define      XFS_SB_VERSION_DIRV2BIT         0x2000
> +#define XFS_SB_VERSION_OLDCIBIT              0x4000
>  #define      XFS_SB_VERSION_MOREBITSBIT      0x8000
>  #define      XFS_SB_VERSION_OKSASHFBITS      \
>       (XFS_SB_VERSION_EXTFLGBIT | \
> -      XFS_SB_VERSION_DIRV2BIT)
> +      XFS_SB_VERSION_DIRV2BIT | \
> +      XFS_SB_VERSION_OLDCIBIT)

there's that oldcibit again...

>  #define      XFS_SB_VERSION_OKREALFBITS      \
>       (XFS_SB_VERSION_ATTRBIT | \
>        XFS_SB_VERSION_NLINKBIT | \
> @@ -77,10 +79,12 @@ struct xfs_mount;
>  #define XFS_SB_VERSION2_LAZYSBCOUNTBIT       0x00000002      /* Superblk 
> counters */
>  #define XFS_SB_VERSION2_RESERVED4BIT 0x00000004
>  #define XFS_SB_VERSION2_ATTR2BIT     0x00000008      /* Inline attr rework */
> +#define XFS_SB_VERSION2_UNICODEBIT   0x00000020      /* Unicode names */
>  
>  #define      XFS_SB_VERSION2_OKREALFBITS     \
>       (XFS_SB_VERSION2_LAZYSBCOUNTBIT | \
> -      XFS_SB_VERSION2_ATTR2BIT)
> +      XFS_SB_VERSION2_ATTR2BIT | \
> +      XFS_SB_VERSION2_UNICODEBIT)
>  #define      XFS_SB_VERSION2_OKSASHFBITS     \
>       (0)
>  #define XFS_SB_VERSION2_OKREALBITS   \
> @@ -145,6 +149,9 @@ typedef struct xfs_sb {
>       __uint16_t      sb_logsectsize; /* sector size for the log, bytes */
>       __uint32_t      sb_logsunit;    /* stripe unit size for the log */
>       __uint32_t      sb_features2;   /* additional feature bits */
> +     __uint32_t      sb_bad_features2; /* features2 could be here */

Ok, so...?

...

> @@ -463,6 +475,12 @@ static inline int xfs_sb_version_hassect
>               ((sbp)->sb_versionnum & XFS_SB_VERSION_SECTORBIT);
>  }
>  
> +static inline int xfs_sb_version_hasoldci(xfs_sb_t *sbp)
> +{
> +     return (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_4) && \
> +             ((sbp)->sb_versionnum & XFS_SB_VERSION_OLDCIBIT);
> +}

You forgot the uppercase indirection macro!   ;)

...


> ===========================================================================
> fs/xfs/xfs_unicode.c
> ===========================================================================
...

> +
> +char *
> +xfs_alloc_unicode_nls_name(void)
> +{
> +     return kmem_zone_alloc(xfs_nls_uni_zone, KM_SLEEP);
> +}

Why wrap/hide this?

> +
> +
> +void
> +xfs_free_unicode_nls_name(
> +     char            *name)
> +{
> +     kmem_zone_free(xfs_nls_uni_zone, name);
> +}

Or this?  Eh, maybe it's handy.

> +int
> +xfs_nls_to_unicode(
> +     struct nls_table *nls,
> +     const char      *nls_name,
> +     int             nls_namelen,
> +     char            **uni_name)
> +{
> +     char            *n;
> +     int             i, o;
> +     wchar_t         uc;
> +     int             nlen;
> +     int             u8len;
> +     int             rval;
> +
> +     n = *uni_name ? *uni_name : xfs_alloc_unicode_nls_name();
> +
> +     if (!nls) {
> +             if (nls_namelen > MAXNAMELEN) {
> +                     rval = -ENAMETOOLONG;

The rest of core xfs code returns positive errors; why the shift in this
file?  Well, I guess because you want to return a length, but this
strikes me as a bit inconsistent... we've been burned by getting error
signs wrong in the past, this looks like an exception to the existing
sign conventions

...


> +int
> +xfs_unicode_validate(
> +     const uchar_t   *name,
> +     int             namelen)
> +{
> +     wchar_t         uc;
> +     int             i, nlen;
> +
> +     for (i = 0; i < namelen; i += nlen) {
> +             if (*name >= 0xf0) {
> +                     cmn_err(CE_WARN, "xfs_unicode_validate: "
> +                                     "UTF-8 char beyond U+FFFF\n");
> +                     return -EINVAL;
> +             }
> +             /* utf8_mbtowc must fail on overlong sequences too */
> +             nlen = utf8_mbtowc(&uc, name + i, namelen - i);
> +             if (nlen < 0) {
> +                     cmn_err(CE_WARN, "xfs_unicode_validate: "
> +                                     "invalid UTF-8 sequence\n");
> +                     return -EILSEQ;
> +             }
> +             /* check for invalid/surrogate/private unicode chars */
> +             if (uc >= 0xfffe || (uc >= 0xd800 && uc <= 0xf8ff)) {
> +                     cmn_err(CE_WARN, "xfs_unicode_validate: "
> +                                     "unsupported UTF-8 char\n");
> +                     return -EINVAL;
> +             }
> +     }
> +     return 0;
> +}

and now this leads to stuff like:

                        rval = xfs_unicode_validate(name, namelen);
                        if (rval < 0)
                                return -rval;

... this looks odd to me.

...

> +xfs_unicode_read_cft(
> +     xfs_mount_t     *mp)
> +{
> +     int             error;
> +     xfs_inode_t     *cftip;
> +     int             size;
> +     int             nfsb;
> +     int             nmap;
> +     xfs_bmbt_irec_t *mapp;
> +     int             n;
> +     int             byte_cnt;
> +     xfs_buf_t       *bp;
> +     char            *table;
> +     xfs_dcft_t      *dcft;
> +
> +     if (mp->m_sb.sb_cftino == NULLFSINO || mp->m_sb.sb_cftino == 0)
> +             return EINVAL;

oh, here it's positive? :)

...

> +void
> +xfs_unicode_free_cft(
> +     const xfs_cft_t *cft)
> +{
> +     remove_cft(cft);
> +}

why the wrapper?

> +void
> +xfs_unicode_init(void)
> +{
> +     mutex_init(&cft_lock);
> +     xfs_nls_uni_zone = kmem_zone_init(MAXNAMELEN, "xfs_nls_uni");
> +}

Hm, no corresponding mutex_destroy

> +void
> +xfs_unicode_uninit(void)
> +{
> +     int             i;
> +
> +     mutex_lock(&cft_lock);
> +
> +     for (i = 0; i < cft_size; i++) {
> +             ASSERT(cft_list[i].refcount == 0);
> +             vfree(cft_list[i].table);
> +     }
> +     kmem_free(cft_list, cft_size * sizeof(struct cft_item));
> +     cft_size = 0;
> +     cft_list = NULL;
> +
> +     mutex_unlock(&cft_lock);

destroy mutex here too just for tidiness

> +     kmem_zone_destroy(xfs_nls_uni_zone);
> +}
> 

...


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