On Wed, Apr 02, 2008 at 04:25:09PM +1000, Barry Naujok wrote:
...
> Index: kern_ci/fs/xfs/xfs_da_btree.h
> ===================================================================
> --- kern_ci.orig/fs/xfs/xfs_da_btree.h
> +++ kern_ci/fs/xfs/xfs_da_btree.h
> @@ -99,6 +99,15 @@ typedef struct xfs_da_node_entry xfs_da_
> *========================================================================*/
>
> /*
> + * Search comparison results
> + */
> +typedef enum {
> + XFS_CMP_DIFFERENT, /* names are completely different */
> + XFS_CMP_EXACT, /* names are exactly the same */
> + XFS_CMP_CASE /* names are same but differ in case */
> +} xfs_dacmp_t;
It is somewhat unfortunate that the "matches" case has multiple values.
memcmp, strcmp, etc. return 0 if the two match, and you make >0 a match, and
0 if they don't. This is really a nitpick, and I don't think there is a way
around...if everyone uses the enum all should be fine.
...
> +/*
> + * Name ops for directory and/or attr name operations
> + */
> +
> +typedef xfs_dahash_t (*xfs_hashname_t)(const uchar_t *, int);
> +typedef xfs_dacmp_t (*xfs_compname_t)(const uchar_t *, int,
> + const uchar_t *, int);
Why have typedefs for function pointers? Sometimes, they even cause problems
(I remember Eric finding a nasty 64-bit bug related to a function pointer
typedef).
Since IRIX isn't on the supported OS list anymore, what's the policy with
coding style within XFS?
...
> Index: kern_ci/fs/xfs/xfs_dir2.h
> ===================================================================
> --- kern_ci.orig/fs/xfs/xfs_dir2.h
> +++ kern_ci/fs/xfs/xfs_dir2.h
> @@ -85,6 +85,12 @@ extern int xfs_dir_canenter(struct xfs_t
> char *name, int namelen);
> extern int xfs_dir_ino_validate(struct xfs_mount *mp, xfs_ino_t ino);
>
> +#define xfs_dir_hashname(dp, n, l) \
> + ((dp)->i_mount->m_dirnameops->hashname((n), (l)))
> +
> +#define xfs_dir_compname(dp, n1, l1, n2, l2) \
> + ((dp)->i_mount->m_dirnameops->compname((n1), (l1), (n2), (l2)))
#define vs. static inline...
I guess this comes back to my question before...what is the coding style
direction you want XFS to go in? More Linux-like (static inline)? or keep it
more IRIX-like (#define)?
...
> --- kern_ci.orig/fs/xfs/xfs_dir2_block.c
> +++ kern_ci/fs/xfs/xfs_dir2_block.c
...
> @@ -698,19 +699,33 @@ xfs_dir2_block_lookup_int(
> ((char *)block + xfs_dir2_dataptr_to_off(mp, addr));
> /*
> * 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
> */
> - 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_da_compname(dep->name, dep->namelen,
> + args->name, args->namelen);
Initial reaction: What's going on here?
if oknoent:
use the mount-determined cmp function
else:
use case-sensitive
That combined with the comment above makes it understandable...but what does
"oknoent" have to do with the whole thing? Wouldn't "exact_match" be a
better name?
Aside from the oknoent rename, I might even turn the ?: into a if-else.
> + if (cmp != XFS_CMP_DIFFERENT && cmp != args->cmpresult) {
> + args->cmpresult = cmp;
> *bpp = bp;
> *entno = mid;
> - return 0;
> + if (cmp == XFS_CMP_EXACT)
> + return 0;
> }
I'd put a comment above the above block...reminding whoever that if you get
XFS_CMP_CASE, you keep scanning to make sure you don't get XFS_CMP_EXACT.
...
> @@ -1391,19 +1394,49 @@ xfs_dir2_leaf_lookup_int(
> xfs_dir2_dataptr_to_off(mp, be32_to_cpu(lep->address)));
> /*
> * If it matches then return it.
> + *
> + * lookup case - use nameops;
> + *
> + * replace/remove case - as lookup has been already been
> + * performed, look for an exact match using the fast method
> */
> - if (dep->namelen == args->namelen &&
> - dep->name[0] == args->name[0] &&
> - memcmp(dep->name, args->name, args->namelen) == 0) {
> - *dbpp = dbp;
> + cmp = args->oknoent ?
> + xfs_dir_compname(dp, dep->name, dep->namelen,
> + args->name, args->namelen) :
> + xfs_da_compname(dep->name, dep->namelen,
> + args->name, args->namelen);
Same as above.
This code is very similar to the above...maybe they should be factored out
in some cleanup patch series.
...
> @@ -578,19 +579,27 @@ xfs_dir2_leafn_lookup_int(
> /*
> * 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_da_compname(dep->name, dep->namelen,
> + args->name, args->namelen);
And again, the same applies. :)
...
> + }
> }
> }
> }
Side note: That's a lot of nesting...yuck :)
Josef 'Jeff' Sipek.
--
UNIX is user-friendly ... it's just selective about who it's friends are
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
