Re: [PATCH v3, 01/16] xfsprogs: metadump: some names aren't all that special

[Dave Chinner <david@fromorbit.com>]

On Fri, Feb 18, 2011 at 03:21:01PM -0600, Alex Elder wrote:
> Move the check for short names out of is_special_dirent() and into
> generate_obfuscated_name().  That way the check is more directly
> associated with the algorithm that requires it.
> 
> Similarly, move the check for inode == 0, since that case has to do
> with storing extended attributes (not files) in the name table.
> 
> As a result, is_special_dirent() is really only focused on whether a
> given file is in the lost+found directory.  Rename the function to
> reflect its more specific purpose.
> 
> Signed-off-by: Alex Elder <aelder@sgi.com>
> 
> Updated:
> - The previous version did not properly skip the "lost+found"
>   directory itself; this one does.
> - Created a new definition representing the name of the orphanage
>   directory.  Encapsulate recognizing that directory into a new
>   macro, is_lost_found().
> - Removed casts that eliminate a compile warning in calls to
>   libxfs_da_hashname(); will do them separately later if needed.
> 
> ---
>  db/metadump.c |   76 +++++++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 52 insertions(+), 24 deletions(-)
> 
> Index: b/db/metadump.c
> ===================================================================
> --- a/db/metadump.c
> +++ b/db/metadump.c
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (c) 2007 Silicon Graphics, Inc.
> + * Copyright (c) 2007, 2011 SGI
>   * All Rights Reserved.
>   *
>   * This program is free software; you can redistribute it and/or
> @@ -377,40 +377,56 @@ random_filename_char(void)
>  	return c;
>  }
>  
> +/*
> + * Determine whether a name is one we shouldn't obfuscate because
> + * it's an orphan (or the "lost+found" directory itself).  Note
> + * "cur_ino" is the inode for the directory currently being
> + * processed.
> + *
> + * Returns 1 if the name should NOT be obfuscated or 0 otherwise.
> + */
> +#define	is_lost_found(mnt, dir_ino, nmlen, nm)			\
> +		((dir_ino) == (mnt)->m_sb.sb_rootino &&		\
> +			(nmlen) == ORPHANAGE_LEN &&		\
> +			!memcmp((nm), ORPHANAGE, ORPHANAGE_LEN))

Perhaps a static inline function?

> +
> +#define	ORPHANAGE	"lost+found"
> +#define	ORPHANAGE_LEN	(sizeof ORPHANAGE - 1)

sizeof works without ()? Even it is does, it is unusual to do so,
and a little ambiguous....

> +
>  static int
> -is_special_dirent(
> +in_lost_found(

Oh, that confused me for a second - in_lost_found and is_lost_found
are very similar in name, hence easily confused when scanning the
code. Not sure how better to name them, maybe you've got a better
idea, Alex?

>  	xfs_ino_t		ino,
>  	int			namelen,
>  	uchar_t			*name)
>  {
>  	static xfs_ino_t	orphanage_ino = 0;
> -	char			s[32];
> +	char			s[24];	/* 21 is enough */

Why is 21 enough?

>  	int			slen;
>  
> -	/*
> -	 * due to the XFS name hashing algorithm, we cannot obfuscate
> -	 * names with 4 chars or less.
> -	 */
> -	if (namelen <= 4)
> -		return 1;
> +	/* Record the "lost+found" inode if we haven't done so already */
>  
> -	if (ino == 0)
> +	ASSERT(ino != 0);
> +	if (!orphanage_ino && is_lost_found(mp, cur_ino, namelen, name))
> +		orphanage_ino = ino;
> +
> +	/* We don't obfuscate the "lost+found" directory itself */
> +
> +	if (ino == orphanage_ino)
> +	    	return 1;
> +
> +	/* Most files aren't in "lost+found" at all */
> +
> +	if (cur_ino != orphanage_ino)
>  		return 0;

I'm judging by this that if a directory tree is attached to
lost+found we are obfuscating anything in that subdirectory?

>  
>  	/*
> -	 * don't obfuscate lost+found nor any inodes within lost+found with
> -	 * the inode number
> +	 * Within "lost+found", we don't obfuscate any file whose
> +	 * name is the same as its inode number.  Any others are
> +	 * stray files and can be obfuscated.
>  	 */
> -	if (cur_ino == mp->m_sb.sb_rootino && namelen == 10 &&
> -			memcmp(name, "lost+found", 10) == 0) {
> -		orphanage_ino = ino;
> -		return 1;
> -	}
> -	if (cur_ino != orphanage_ino)
> -		return 0;
> +	slen = snprintf(s, sizeof s, "%llu", (unsigned long long) ino);
>  
> -	slen = sprintf(s, "%lld", (long long)ino);
> -	return (slen == namelen && memcmp(name, s, namelen) == 0);
> +	return slen == namelen && !memcmp(name, s, namelen);
>  }
>  
>  static void
> @@ -426,13 +442,25 @@ generate_obfuscated_name(
>  	xfs_dahash_t		newhash;
>  	uchar_t			newname[NAME_MAX];
>  
> -	if (is_special_dirent(ino, namelen, name))
> -		return;
> +	/*
> +	 * Our obfuscation algorithm requires at least 5-character
> +	 * names, so don't bother if the name is too short.
> +	 */
> +	if (namelen < 5)
> +	    	return;

Please make usre you include the reason for this - that this is a
property of the name hashing algorithm.

> -	hash = libxfs_da_hashname(name, namelen);
> +	/*
> +	 * We don't obfuscate "lost+found" or any orphan files
> +	 * therein.  When the name table is used for extended
> +	 * attributes, the inode number provided is 0, in which
> +	 * case we don't need to make this check.
> +	 */
> +	if (ino && in_lost_found(ino, namelen, name))
> +		return;
>  
>  	/* create a random name with the same hash value */
>  
> +	hash = libxfs_da_hashname(name, namelen);
>  	do {
>  		dup = 0;
>  		newname[0] = '/';
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs