Re: EXT2FS Problems -- Interesting Code Snippet
> I realized that the error message I was reporting is actually a bit
> different. Rerunning the system I see that the message is actually:
>
> "ext2fs: Warning: bad type XX in directory entry: inode: YY offset: ZZ"
For future reference, it often really is more helpful to give the exact
numbers in the message you saw. In this case, YY is an inode number and ZZ
a offset, but XX is a type code that should be one of a small constant set,
and so what the bogus value is might actually be telling.
At any rate, I think I have fixed the bug that causes this warning message.
The bug resulted from my update to support the new file_type field in
directory entries. I updated the reading code to the new format, which
also works fine for old-format directories (because the file_type field
lies in the high byte of the name length field, which is always zero in an
old directory). However, I failed to update the directory writing code,
though it was using the new structure definition for directory entries.
The result was that the file_type field (or high byte of the length field,
as considered by an older ext2fs) in newly-written directory entries was
uninitialized stack garbage.
The patch below (already checked in) fixes the bug by initializing the
file_type field in directory entries properly (according to the setting of
the feature flag in the superblock). (It's a very simple change really.
The patch is a bit longer because I rearranged some of the code to be
easier to maintain.)
> So the error message we are seeing might just be due to the
> new warning code added. However, the fact that I experience
> real data corruption as well as this message indicates that
> we are picking up an actual error.
I am pretty confident about the nature and ramifications of this particular
bug. I don't think that there could be any result other than the "bad
type" warnings from this particular bug. The other errors that have been
reported suggesting data corruption are more likely an unrelated bug,
probably introduced around the same time by my other untested ext2fs format
support changes. Do please keep looking for more information on the data
corruption problems.
> As a side note, shouldn't we be using memmove rather than
> memcpy in the last line added (above)? The bcopy we
> are replacing correctly handles cases where the source and
> destination overlap, while memcpy does not. Perhaps you
> are concerned that memmove would have a performance impact?
> I suppose it is unlikely that these two regions of memory
> ever would overlap.
It is not only unlikely but certain in this case. Since bcopy and memmove
have to handle overlap, they may be more or slower code; it is always
preferable to use memcpy when it suffices. The possibility that two given
memory blocks that might overlap is an unusual situation that the
programmer better already be aware of and either specifically have in mind
allowing and coping with, or presume is ruled out. So using memcpy as the
default as a matter of course is best; if there is any possibility memmove
is required, then you had better have thought about the ramifications of
that possibility in detail already.
Here's the patch for the directory bug:
1999-10-03 Roland McGrath <roland@baalperazim.frob.com>
* dir.c (file_type_ext2): New const variable, map DT_* -> EXT2_FT_*.
(diskfs_direnter_hard): Move initialization of directory entry content
fields out of switch; use memcpy or memmove as appropriate, instead of
bcopy. Set file_type field in new directory entry to appropriate
type for the node, or to zero if the filesystem doesn't have the
EXT2_FEATURE_INCOMPAT_FILETYPE flag set.
Index: dir.c
===================================================================
RCS file: /afs/sipb.mit.edu/project/hurddev/cvsroot/hurd/ext2fs/dir.c,v
retrieving revision 1.33
retrieving revision 1.34
diff -u -b -p -r1.33 -r1.34
--- dir.c 1999/09/13 06:33:02 1.33
+++ dir.c 1999/10/03 05:05:09 1.34
@@ -101,6 +101,30 @@ dirscanblock (vm_address_t blockoff, str
const char *name, int namelen, enum lookup_type type,
struct dirstat *ds, ino_t *inum);
+
+static const unsigned char ext2_file_type[EXT2_FT_MAX] =
+{
+ [EXT2_FT_UNKNOWN] = DT_UNKNOWN,
+ [EXT2_FT_REG_FILE] = DT_REG,
+ [EXT2_FT_DIR] = DT_DIR,
+ [EXT2_FT_CHRDEV] = DT_CHR,
+ [EXT2_FT_BLKDEV] = DT_BLK,
+ [EXT2_FT_FIFO] = DT_FIFO,
+ [EXT2_FT_SOCK] = DT_SOCK,
+ [EXT2_FT_SYMLINK] = DT_LNK,
+};
+static const unsigned char file_type_ext2[] =
+{
+ [DT_UNKNOWN] = EXT2_FT_UNKNOWN,
+ [DT_REG] = EXT2_FT_REG_FILE,
+ [DT_DIR] = EXT2_FT_DIR,
+ [DT_CHR] = EXT2_FT_CHRDEV,
+ [DT_BLK] = EXT2_FT_BLKDEV,
+ [DT_FIFO] = EXT2_FT_FIFO,
+ [DT_SOCK] = EXT2_FT_SOCK,
+ [DT_LNK] = EXT2_FT_SYMLINK,
+};
+
/* Implement the diskfs_lookup from the diskfs library. See
<hurd/diskfs.h> for the interface specification. */
error_t
@@ -509,16 +533,17 @@ diskfs_direnter_hard (struct node *dp, c
dp->dn_set_mtime = 1;
+ /* Select a location for the new directory entry. Each branch of this
+ switch is responsible for setting NEW to point to the on-disk
+ directory entry being written, and setting NEW->rec_len appropriately. */
+
switch (ds->stat)
{
case TAKE:
/* We are supposed to consume this slot. */
assert (ds->entry->inode == 0 && ds->entry->rec_len >= needed);
-
- ds->entry->inode = np->cache_id;
- ds->entry->name_len = namelen;
- bcopy (name, ds->entry->name, namelen);
+ new = ds->entry;
break;
case SHRINK:
@@ -529,13 +554,8 @@ diskfs_direnter_hard (struct node *dp, c
new = (struct ext2_dir_entry_2 *) ((vm_address_t) ds->entry + oldneeded);
- new->inode = np->cache_id;
new->rec_len = ds->entry->rec_len - oldneeded;
- new->name_len = namelen;
- bcopy (name, new->name, namelen);
-
ds->entry->rec_len = oldneeded;
-
break;
case COMPRESS:
@@ -555,7 +575,7 @@ diskfs_direnter_hard (struct node *dp, c
{
assert (fromoff >= tooff);
- bcopy (from, to, fromreclen);
+ memmove (to, from, fromreclen);
to->rec_len = EXT2_DIR_REC_LEN (to->name_len);
tooff += to->rec_len;
@@ -567,10 +587,7 @@ diskfs_direnter_hard (struct node *dp, c
assert (totfreed >= needed);
new = (struct ext2_dir_entry_2 *) tooff;
- new->inode = np->cache_id;
new->rec_len = totfreed;
- new->name_len = namelen;
- bcopy (name, new->name, namelen);
break;
case EXTEND:
@@ -593,18 +610,28 @@ diskfs_direnter_hard (struct node *dp, c
dp->dn_stat.st_size = oldsize + DIRBLKSIZ;
dp->dn_set_ctime = 1;
- new->inode = np->cache_id;
new->rec_len = DIRBLKSIZ;
- new->name_len = namelen;
- bcopy (name, new->name, namelen);
break;
default:
- assert (0);
+ new = 0;
+ assert (! "impossible: bogus status field in dirstat");
}
- dp->dn_set_mtime = 1;
+ /* NEW points to the directory entry being written, and its
+ rec_len field is already filled in. Now fill in the rest. */
+
+ new->inode = np->cache_id;
+ new->file_type = (EXT2_HAS_INCOMPAT_FEATURE (sblock,
+ EXT2_FEATURE_INCOMPAT_FILETYPE)
+ ? file_type_ext2[IFTODT (np->dn_stat.st_mode & S_IFMT)]
+ : 0);
+ new->name_len = namelen;
+ memcpy (new->name, name, namelen);
+
+ /* Mark the directory inode has having been written. */
dp->dn->info.i_flags &= ~EXT2_BTREE_FL;
+ dp->dn_set_mtime = 1;
munmap ((caddr_t) ds->mapbuf, ds->mapextent);
@@ -810,19 +837,6 @@ count_dirents (struct node *dp, int nb,
/* Returned directory entries are aligned to blocks this many bytes long.
Must be a power of two. */
#define DIRENT_ALIGN 4
-
-static const unsigned char ext2_file_type[EXT2_FT_MAX] =
-{
- [EXT2_FT_UNKNOWN] = DT_UNKNOWN,
- [EXT2_FT_REG_FILE] = DT_REG,
- [EXT2_FT_DIR] = DT_DIR,
- [EXT2_FT_CHRDEV] = DT_CHR,
- [EXT2_FT_BLKDEV] = DT_BLK,
- [EXT2_FT_FIFO] = DT_FIFO,
- [EXT2_FT_SOCK] = DT_SOCK,
- [EXT2_FT_SYMLINK] = DT_LNK,
-};
-
/* Implement the disikfs_get_directs callback as described in
<hurd/diskfs.h>. */
Reply to: