xfs
[Top] [All Lists]

Re: [PATCH v3, 13/16] xfsprogs: metadump: move duplicate name handling i

To: Alex Elder <aelder@xxxxxxx>
Subject: Re: [PATCH v3, 13/16] xfsprogs: metadump: move duplicate name handling into its own function
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Thu, 24 Feb 2011 13:12:39 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <201102182121.p1ILL2pI029171@xxxxxxxxxxxxxxxxxxxxxx>
References: <201102182121.p1ILL2pI029171@xxxxxxxxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.20 (2009-06-14)
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@xxxxxxx>
> 
> 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@xxxxxxxxxxxxx

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