xfs
[Top] [All Lists]

[PATCH 4/4] xfs_repair: remove last-entry hack in process_sf_dir2

To: xfs-oss <xfs@xxxxxxxxxxx>
Subject: [PATCH 4/4] xfs_repair: remove last-entry hack in process_sf_dir2
From: Eric Sandeen <sandeen@xxxxxxxxxxx>
Date: Tue, 10 Mar 2015 17:11:38 -0400
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <54FF59DA.60700@xxxxxxxxxxx>
References: <54FF59DA.60700@xxxxxxxxxxx>
process_sf_dir2() tries to special-case the last entry in
a short form dir, and salvage it if the name length is wrong
by using the dir size as a clue to what the length should be.

However, this doesn't actually work; there is either a 32-bit
or 64-bit inode after the name, and with dirv3 there's a file
type in there as well.  Extending the name length to the dir
size means it overlaps these fields, which often have a 0 in
the top bits, and then namecheck() fails the result and junks
it anyway:

> entry #1 is zero length in shortform dir 67, resetting to 29
> entry contains illegal character in shortform dir 67
> junking entry "bbbbbbbbbbbbbbbbbbbbbbbb" in directory inode 67

And depending on the corruption, the current code will set
a new negative namelen if it turns out that the name itself
starts beyond dir size; it can't be shortened enough.

So, we could fix this up, and choose a new namelen such that
the xfs_dir2_ino8_t and/or xfs_dir2_ino8_t and/or file type
still fits at the end, but we really seem to be reaching the
point of diminishing returns.  The chances that nothing but
the name length is wrong, and that the remaining few
bytes at the end of the dir size are actually correct, seems
awfully small.

Therefore just remove this special case, which I think is
of questionable value.

Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
---

diff --git a/repair/dir2.c b/repair/dir2.c
index 5512e41..b1816f4 100644
--- a/repair/dir2.c
+++ b/repair/dir2.c
@@ -874,43 +874,20 @@ _("entry \"%*.*s\" in shortform directory %" PRIu64 " 
references %s inode %" PRI
                }
 
                if (bad_sfnamelen) {
+                       do_warn(
+_("entry #%d %s in shortform dir %" PRIu64),
+                               i, junkreason, ino);
+                       if (!no_modify)
+                               do_warn(_(", junking %d entries\n"),
+                                       num_entries - i);
+                       else
+                               do_warn(_(", would junk %d entries\n"),
+                                       num_entries - i);
                        /*
-                        * if we're really lucky, this is the last entry in
-                        * case we can use the dir size to set the namelen
-                        * value.  otherwise, forget it because we're not going
-                        * to be able to find the next entry.
+                        * don't process the rest of the directory,
+                        * break out of processing loop
                         */
-                       if (i == num_entries - 1)  {
-                               namelen = ino_dir_size -
-                                       ((__psint_t) &sfep->name[0] -
-                                        (__psint_t) sfp);
-                               if (!no_modify)  {
-                                       do_warn(
-_("entry #%d %s in shortform dir %" PRIu64 ", resetting to %d\n"),
-                                               i, junkreason, ino, namelen);
-                                       sfep->namelen = namelen;
-                                       *dino_dirty = 1;
-                               } else  {
-                                       do_warn(
-_("entry #%d %s in shortform dir %" PRIu64 ", would set to %d\n"),
-                                               i, junkreason, ino, namelen);
-                               }
-                       } else  {
-                               do_warn(
-_("entry #%d %s in shortform dir %" PRIu64),
-                                       i, junkreason, ino);
-                               if (!no_modify)
-                                       do_warn(_(", junking %d entries\n"),
-                                               num_entries - i);
-                               else
-                                       do_warn(_(", would junk %d entries\n"),
-                                               num_entries - i);
-                               /*
-                                * don't process the rest of the directory,
-                                * break out of processing looop
-                                */
-                               break;
-                       }
+                       break;
                }
 
                /*

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