Re: [PATCH v3, 13/16] xfsprogs: metadump: move duplicate name handling into its own function

[Dave Chinner <david@fromorbit.com>]

On Fri, Feb 18, 2011 at 03:21:02PM -0600, Alex Elder wrote:
> Move the handling of duplicate names into its own function.  As a
> result, all names other than "lost+found" files (not just those that
> get obfuscated) will be checked to avoid duplication.
> 
> This makes the local buffer newname[] in generate_obfuscated_name()
> unnecessary, so just drop it and use the passed-in name.
> 
> Signed-off-by: Alex Elder <aelder@sgi.com>
> 
> This is a new change, not posted with this series previously.

Couple of minor comments below.

> 
> ---
>  db/metadump.c |   95 +++++++++++++++++++++++++++++-----------------------------
>  1 file changed, 49 insertions(+), 46 deletions(-)
> 
> Index: b/db/metadump.c
> ===================================================================
> --- a/db/metadump.c
> +++ b/db/metadump.c
> @@ -469,6 +469,37 @@ in_lost_found(
>  }
>  
>  /*
> + * Look up the given name in the name table.  If it is already
> + * present, find an alternate and attempt to use that name instead.
> + *
> + * Returns 1 if the (possibly modified) name is not present in the
> + * name table.  Returns 0 otherwise.
> + */
> +static int
> +handle_duplicates(xfs_dahash_t hash, size_t name_len, uchar_t *name)

handle_duplicate_name()?

> +{
> +	int	dup = 0;
> +
> +	if (!nametable_find(hash, name_len, name))
> +	    	return 1;	/* Not already in table */
> +
> +	/* Name is already in use.  Need to find an alternate. */
> +
> +	do {
> +		obfuscate_name(hash, name_len, name);
> +
> +		/*
> +		 * Search the name table to be sure we don't produce
> +		 * a name that's already been used.
> +		 */
> +		if (!nametable_find(hash, name_len, name))
> +			break;
> +	} while (++dup < DUP_MAX);
> +
> +	return dup < DUP_MAX ? 1 : 0;
> +}
> +
> +/*
>   * Given a name and its hash value, massage the name in such a way
>   * that the result is another name of equal length which shares the
>   * same hash value.
> @@ -549,9 +580,6 @@ generate_obfuscated_name(
>  	uchar_t			*name)
>  {
>  	xfs_dahash_t		hash;
> -	int			dup = 0;
> -	uchar_t			newname[NAME_MAX];
> -	uchar_t			*newp;
>  
>  	/*
>  	 * We don't obfuscate "lost+found" or any orphan files
> @@ -562,58 +590,33 @@ generate_obfuscated_name(
>  	if (ino && in_lost_found(ino, namelen, name))
>  		return;
>  
> -	/*
> -	 * If the name starts with a slash, just skip over it.  We
> -	 * will copy our obfuscated name back into space following
> -	 * the slash when we're done.  Our new name will not have
> -	 * the '/', and that's the version we'll keep in our
> -	 * duplicates table.  Note that the namelen value passed in
> -	 * does not include the leading slash (if any).
> -	 */
>  	if (*name == '/')
> -	    	name++;
> +	    	name++;		/* Skip over leading slash (if any). */

I think the comment explaining _why_ we are skipping over the slash
is much more important than the new comment that just describes what
the code is doing. Hence I'd prefer a slightly modified version of
the old comment rather than removing it altogether...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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