fs: fat: handle deleted directory entries correctly

Unlink test for FAT file system seems to fail at test_unlink2.
(When I added this test, I haven't seen any errors though.)
for example,
===8<===
fs_obj_unlink = ['fat', '/home/akashi/tmp/uboot_sandbox_test/128MB.fat32.img']

    def test_unlink2(self, u_boot_console, fs_obj_unlink):
        """
        Test Case 2 - delete many files
        """
        fs_type,fs_img = fs_obj_unlink
        with u_boot_console.log.section('Test Case 2 - unlink (many)'):
            output = u_boot_console.run_command('host bind 0 %s' % fs_img)

            for i in range(0, 20):
                output = u_boot_console.run_command_list([
                    '%srm host 0:0 dir2/0123456789abcdef%02x' % (fs_type, i),
                    '%sls host 0:0 dir2/0123456789abcdef%02x' % (fs_type, i)])
                assert('' == ''.join(output))

            output = u_boot_console.run_command(
                '%sls host 0:0 dir2' % fs_type)
>           assert('0 file(s), 2 dir(s)' in output)
E           AssertionError: assert '0 file(s), 2 dir(s)' in '            ./\r\r\n            ../\r\r\n        0   0123456789abcdef11\r\r\n\r\r\n1 file(s), 2 dir(s)'

test/py/tests/test_fs/test_unlink.py:52: AssertionError
===>8===

This can happen when fat_itr_next() wrongly detects an already-
deleted directory entry.

File deletion, which was added in the commit f8240ce95d64 ("fs: fat:
support unlink"), is implemented by marking its entry for a short name
with DELETED_FLAG, but related entry slots for a long file name are kept
unmodified. (So entries will never be actually deleted from media.)

To handle this case correctly, an additional check for a directory slot
will be needed in fat_itr_next().

In addition, I added extra comments about long file name and short file
name format in FAT file system. Although they are not directly related
to the issue, I hope it will be helpful for better understandings
in general.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index 9e1b842dac6b9802960b19bb180a90e894679159..68ce658386780af4494ff309c4cd0136080484b3 100644 (file)
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -869,6 +869,14 @@ static dir_entry *extract_vfat_name(fat_itr *itr)
                        return NULL;
        }
 
                        return NULL;
        }
 

+       /*
+        * We are now at the short file name entry.
+        * If it is marked as deleted, just skip it.
+        */
+       if (dent->name[0] == DELETED_FLAG ||
+           dent->name[0] == aRING)
+               return NULL;
+

        itr->l_name[n] = '\0';
 
        chksum = mkcksum(dent->name, dent->ext);
        itr->l_name[n] = '\0';
 
        chksum = mkcksum(dent->name, dent->ext);


@@ -898,6 +906,16 @@ static int fat_itr_next(fat_itr *itr)
 
        itr->name = NULL;
 
 
        itr->name = NULL;
 

+       /*
+        * One logical directory entry consist of following slots:
+        *                              name[0] Attributes
+        *   dent[N - N]: LFN[N - 1]    N|0x40  ATTR_VFAT
+        *   ...
+        *   dent[N - 2]: LFN[1]        2       ATTR_VFAT
+        *   dent[N - 1]: LFN[0]        1       ATTR_VFAT
+        *   dent[N]:     SFN                   ATTR_ARCH
+        */
+

        while (1) {
                dent = next_dent(itr);
                if (!dent)
        while (1) {
                dent = next_dent(itr);
                if (!dent)


@@ -910,7 +928,17 @@ static int fat_itr_next(fat_itr *itr)
                if (dent->attr & ATTR_VOLUME) {
                        if ((dent->attr & ATTR_VFAT) == ATTR_VFAT &&
                            (dent->name[0] & LAST_LONG_ENTRY_MASK)) {
                if (dent->attr & ATTR_VOLUME) {
                        if ((dent->attr & ATTR_VFAT) == ATTR_VFAT &&
                            (dent->name[0] & LAST_LONG_ENTRY_MASK)) {

+                               /* long file name */

                                dent = extract_vfat_name(itr);
                                dent = extract_vfat_name(itr);

+                               /*
+                                * If succeeded, dent has a valid short file
+                                * name entry for the current entry.
+                                * If failed, itr points to a current bogus
+                                * entry. So after fetching a next one,
+                                * it may have a short file name entry
+                                * for this bogus entry so that we can still
+                                * check for a short name.
+                                */

                                if (!dent)
                                        continue;
                                itr->name = itr->l_name;
                                if (!dent)
                                        continue;
                                itr->name = itr->l_name;


@@ -919,8 +947,11 @@ static int fat_itr_next(fat_itr *itr)
                                /* Volume label or VFAT entry, skip */
                                continue;
                        }
                                /* Volume label or VFAT entry, skip */
                                continue;
                        }

-               }
+               } else if (!(dent->attr & ATTR_ARCH) &&
+                          !(dent->attr & ATTR_DIR))
+                       continue;

 
 

+               /* short file name */

                break;
        }
 
                break;
        }