xfsprogs: metadump: some names aren't all that special
PATCH v3 1/16 xfsprogs: metadump: some names aren't all that special
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.
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(-)
db/metadump.c +52 -24
@@ -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)) + +#define ORPHANAGE "lost+found" +#define ORPHANAGE_LEN (sizeof ORPHANAGE - 1) +static int -is_special_dirent ( +in_lost_found (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 */ 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 ;/* -* 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 ; - 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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 43 lines · Alex Elder
Move the check for short names out of
is_special_dirent()and intogenerate_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 <[email protected]>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 tolibxfs_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
===================================================================db/metadump.c +3
-1@@ -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
db/metadump.c random_filename_char(void) · line 391
Quoted 10 lines
db/metadump.c +10
-0+* 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?
db/metadump.c random_filename_char(void) · line 394
Quoted 3 lines
db/metadump.c +3
-0+ +#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....
db/metadump.c random_filename_char(void) · line 397
Quoted 4 lines
db/metadump.c +2
-1+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?
db/metadump.c random_filename_char(void) · line 403
Quoted 7 lines
db/metadump.c +1
-1xfs_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?
Quoted 14 lines
db/metadump.c +4
-7int 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;
db/metadump.c random_filename_char(void) · line 420
Quoted 10 lines
db/metadump.c +9
-0+ +/* 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?
Quoted 25 lines
db/metadump.c +5
-11/* -* 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;
db/metadump.c generate_obfuscated_name( · line 450
Quoted 10 lines
db/metadump.c +6
-2uchar_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.
Quoted 22 lines
db/metadump.c +9
-1- 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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 8 lines · Dave Chinner
Quoted 7 lines · Alex Elder
Move the check for short names out of
is_special_dirent()and intogenerate_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.
Quoted 7 lines
Quoted 7 lines
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 <[email protected]>Updated:
. . .
Quoted 6 lines
db/metadump.c random_filename_char(void) · line 391
Quoted 4 lines
db/metadump.c +4
-0+#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?
OK.
Quoted 6 lines
db/metadump.c random_filename_char(void) · line 394
Quoted 3 lines
db/metadump.c +3
-0+ +#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....
Yes it does. You need the parentheses when you're asking about a type name, but for an object they are not needed. It is not ambiguous. Nevertheless I don't mind adding two characters to the patch.
Quoted 9 lines
db/metadump.c random_filename_char(void) · line 397
Quoted 4 lines
db/metadump.c +2
-1+static int -is_special_dirent( +in_lost_found(Oh, that confused me for a second -
in_lost_foundandis_lost_foundare 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?
I had the same thought, actually, but didn't do anything about it. I could use is_orphanage_dir(), what do you think of that? Or alternately could change in_lost_found() to be is_orphan() (or both). Unless I hear a better suggestion I'll just do is_orphanage_dir(), as an inline function.
Quoted 10 lines
db/metadump.c random_filename_char(void) · line 403
Quoted 8 lines
db/metadump.c +1
-1xfs_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?
Because it's formatting a 64-bit unsigned in
decimal. 2^64 = 18 446 744 073 709 551 616
That's 20 digits, plus a trailing '\0'. Do
you want me to clarify this in a comment
somehow? (I suppose unsigned long long is
not technically guaranteed to be 64 bits
either.)
Quoted 20 lines
Quoted 20 lines
db/metadump.c +9
-7int 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 ;
Quoted 8 lines
db/metadump.c random_filename_char(void) · line 420
Quoted 5 lines
db/metadump.c +4
-0+ +/* 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?
Yes. I preserved the way the code worked before, which is that:
- lost+found itself is not obfuscated
- files directly under lost+found whose filename is the decimal representation of the file's inode number are treated as orphans, and they are not obfuscated
- anything else (including things in lost+found with non-inode-number names, and anything below a subdirectory under lost+found) is obfuscated.
Quoted 27 lines
Quoted 27 lines
db/metadump.c +5
-11/* -* 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 ];
Quoted 11 lines
db/metadump.c generate_obfuscated_name( · line 450
Quoted 8 lines
db/metadump.c +6
-2-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.
Well, the comment above it alludes to it, although it emphasizes obfuscation rather than the hash. I'll try to come up with a concise way to do what you ask though.
Quoted 10 lines
Quoted 10 lines
db/metadump.c +8
-1- 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 ;
Quoted 7 lines
db/metadump.c generate_obfuscated_name(
Quoted 7 lines
db/metadump.c +1
-0/* create a random name with the same hash value */ + hash =libxfs_da_hashname (name, namelen); do { dup = 0; newname[0] = '/';
I'll adjust my patch based on your comments and re-post for a final review. I'll fix and re-post the few other patches you had suggestions on too.
Thanks a lot for reviewing it, Dave.
-Alex
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH v4 1/16 xfsprogs: metadump: some names aren't all that special
In reply to Dave ChinnerMove 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.
Updates (v3):
- 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.
Updates (v4):
- Renamed is_lost_found() to be is_orphanage_dir(), and turned it into an inline static function.
- Added parentheses around targets of the sizeof operation.
- Added a small bit of clarifying commentary in spots where it was suggested.
--- db/metadump.c | 86 +++++++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 62 insertions(+), 24 deletions(-)
db/metadump.c +62 -24
@@ -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,63 @@ random_filename_char(void)return c; } +#define ORPHANAGE "lost+found" +#define ORPHANAGE_LEN (sizeof (ORPHANAGE) - 1) + +static inline int +is_orphanage_dir ( +struct xfs_mount *mp, +xfs_ino_t dir_ino, +size_t name_len, +uchar_t *name) +{ +return dir_ino == mp->m_sb.sb_rootino && + name_len ==ORPHANAGE_LEN && + !memcmp (name,ORPHANAGE ,ORPHANAGE_LEN ); +} + +/* +* 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. +*/ static int -is_special_dirent ( +in_lost_found (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 (64 bits in decimal) */ 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_orphanage_dir (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 ;/* -* 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 +449,28 @@ 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. We +* work backward from a hash value to determine the last +* five bytes in a name required to produce a new name +* with the same hash. +*/ +if (namelen <5 ) +return ; - 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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 13 lines · Alex Elder
Move the check for short names out of
is_special_dirent()and intogenerate_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 <[email protected]>
Quoted 14 lines
Updates (v3):
- 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 tolibxfs_da_hashname(); will do them separately later if needed.
Updates (v4):
- Renamedis_lost_found()to beis_orphanage_dir(), and turned it into an inline static function.
- Added parentheses around targets of the sizeof operation.
- Added a small bit of clarifying commentary in spots where it was suggested.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs