Readdir fixes, ext2_readdir()
authorBarret Rhoden <brho@cs.berkeley.edu>
Wed, 8 Sep 2010 00:18:03 +0000 (17:18 -0700)
committerKevin Klues <klueska@cs.berkeley.edu>
Thu, 3 Nov 2011 00:35:53 +0000 (17:35 -0700)
Ext2 directories can now be read / ls'd.  Note we are leaking inodes.

This also decouples d_off from the implementation of generic_dir_read(),
such that it's an FS-internal hint.  Also fixes a couple bugs and allows
"fs ls" to cross mount points.

kern/src/ext2fs.c
kern/src/kfs.c
kern/src/vfs.c

index e07b910..29e0bd4 100644 (file)
@@ -12,6 +12,7 @@
 #include <assert.h>
 #include <kref.h>
 #include <endian.h>
+#include <error.h>
 
 /* These structs are declared again and initialized farther down */
 struct page_map_operations ext2_pm_op;
@@ -369,7 +370,13 @@ struct inode *ext2_alloc_inode(struct super_block *sb)
 /* deallocs and cleans up after an inode. */
 void ext2_dealloc_inode(struct inode *inode)
 {
-I_AM_HERE;
+       static bool ran_once = FALSE;
+       if (!ran_once) {
+               ran_once = TRUE;
+               warn("Implement %s(), you're leaking memory!\n", __FUNCTION__);
+       }
+/* too verbose, but it's a TODO... */
+//I_AM_HERE;
        #if 0
        /* If we're a symlink, give up our storage for the symname */
        if (S_ISLNK(inode->i_mode))
@@ -760,29 +767,39 @@ I_AM_HERE;
 
 /* Fills in the next directory entry (dirent), starting with d_off.  Like with
  * read and write, there will be issues with userspace and the *dirent buf.
- * TODO: we don't really do anything with userspace concerns here, in part
- * because memcpy_to doesn't work well.  When we fix how we want to handle the
- * userbuffers, we can write this accordingly. (UMEM)  */
+ * TODO: (UMEM) */
 int ext2_readdir(struct file *dir, struct dirent *dirent)
 {
-       I_AM_HERE;
-       #if 0
-       int count = 0;
-       /* some of this error handling can be done by the VFS.  The syscall should
-        * handle EBADF, EFAULT, and EINVAL (TODO, memory related). */
-       if (!S_ISDIR(dir_d->d_inode->i_mode)) {
-               set_errno(ENOTDIR);
-               return -1;
-       }
-
-
-       if (!found) {
-               set_errno(ENOENT);
-               return -1;
-       }
-       if (count - 1 == dirent->d_off)         /* found the last dir in the list */
+       void *buffer;
+       /* Not enough data at the end of the directory */
+       if (dir->f_dentry->d_inode->i_size <
+           dirent->d_off + sizeof(struct ext2_dirent))
+               return -ENOENT;
+       
+       /* Figure out which block we need to read in for dirent->d_off */
+       int block = dirent->d_off / dir->f_dentry->d_sb->s_blocksize;
+       buffer = ext2_read_ino_block(dir->f_dentry->d_inode, block);
+       assert(buffer);
+       off_t f_off = dirent->d_off % dir->f_dentry->d_sb->s_blocksize;
+       /* Copy out the dirent info */
+       struct ext2_dirent *e2dir = (struct ext2_dirent*)(buffer + f_off);
+       dirent->d_ino = le32_to_cpu(e2dir->dir_inode);
+       dirent->d_off += le16_to_cpu(e2dir->dir_reclen);
+       /* note, dir_namelen doesn't include the \0 */
+       dirent->d_reclen = e2dir->dir_namelen;
+       strncpy(dirent->d_name, (char*)e2dir->dir_name, e2dir->dir_namelen);
+       assert(e2dir->dir_namelen <= MAX_FILENAME_SZ);
+       dirent->d_name[e2dir->dir_namelen] = '\0';
+       kfree(buffer);
+       
+       /* At the end of the directory, sort of.  ext2 often preallocates blocks, so
+        * this will cause us to walk along til the end, which isn't quite right. */
+       if (dir->f_dentry->d_inode->i_size == dirent->d_off)
                return 0;
-       #endif
+       if (dir->f_dentry->d_inode->i_size < dirent->d_off) {
+               warn("Issues reaching the end of an ext2 directory!");
+               return 0;
+       }
        return 1;                                                       /* normal success for readdir */
 }
 
@@ -802,7 +819,8 @@ int ext2_mmap(struct file *file, struct vm_region *vmr)
  * the FS to do whatever it needs. */
 int ext2_open(struct inode *inode, struct file *file)
 {
-       I_AM_HERE;
+       /* TODO: check to make sure the file is openable, and maybe do some checks
+        * for the open mode (like did we want to truncate, append, etc) */
        return 0;
 }
 
index 61ca4c9..61685e0 100644 (file)
@@ -539,7 +539,8 @@ off_t kfs_llseek(struct file *file, off_t offset, int whence)
 }
 
 /* Fills in the next directory entry (dirent), starting with d_off.  KFS treats
- * the size of each dirent as one byte.
+ * the size of each dirent as 1 byte, which we can get away with since the d_off
+ * is a way of communicating with future calls to readdir (FS-specific).
  *
  * Like with read and write, there will be issues with userspace and the *dirent
  * buf.  TODO: we don't really do anything with userspace concerns here, in part
@@ -548,7 +549,7 @@ off_t kfs_llseek(struct file *file, off_t offset, int whence)
 int kfs_readdir(struct file *dir, struct dirent *dirent)
 {
        int count = 2;  /* total num dirents, gets incremented in check_entry() */
-       off_t desired_off = dirent->d_off;
+       int desired_file = dirent->d_off;
        bool found = FALSE;
        struct dentry *subent;
        struct dentry *dir_d = dir->f_dentry;
@@ -557,7 +558,7 @@ int kfs_readdir(struct file *dir, struct dirent *dirent)
        /* how we check inside the for loops below.  moderately ghetto. */
        void check_entry(void)
        {
-               if (count++ == desired_off) {
+               if (count++ == desired_file) {
                        dirent->d_ino = subent->d_inode->i_ino;
                        dirent->d_off = count;
                        dirent->d_reclen = subent->d_name.len;
@@ -567,38 +568,32 @@ int kfs_readdir(struct file *dir, struct dirent *dirent)
                        found = TRUE;
                }
        }
-       /* some of this error handling can be done by the VFS.  The syscall should
-        * handle EBADF, EFAULT, and EINVAL (TODO, memory related). */
-       if (!S_ISDIR(dir_d->d_inode->i_mode)) {
-               set_errno(ENOTDIR);
-               return -1;
-       }
 
        /* Handle . and .. (first two dirents) */
-       if (dirent->d_off == 0) {
+       if (desired_file == 0) {
                dirent->d_ino = dir_d->d_inode->i_ino;
                dirent->d_off = 1;
                dirent->d_reclen = 1;
-               strncpy(dirent->d_name, ".", 1);
+               strncpy(dirent->d_name, ".", 2);        /* the extra is for the null term */
                found = TRUE;
-       } else if (dirent->d_off == 1) {
+       } else if (desired_file == 1) {
                dirent->d_ino = dir_d->d_parent->d_inode->i_ino;
                dirent->d_off = 2;
                dirent->d_reclen = 2;
-               strncpy(dirent->d_name, "..", 2);
+               strncpy(dirent->d_name, "..", 3);       /* the extra is for the null term */
                found = TRUE;
-       } else {
-               /* need to check the sub-dirs as well as the sub-"files" */
-               TAILQ_FOREACH(subent, &dir_d->d_subdirs, d_subdirs_link)
-                       check_entry();
-               TAILQ_FOREACH(subent, &k_i_info->children, d_subdirs_link)
-                       check_entry();
-       }
-       if (!found) {
-               set_errno(ENOENT);
-               return -1;
        }
-       if (count == dirent->d_off)             /* found the last dir in the list */
+       /* need to check the sub-dirs as well as the sub-"files".  The main
+        * ghetto-ness with this is that we check even though we have our result,
+        * simply to figure out how big our directory is.  It's just not worth
+        * changing at this point. */
+       TAILQ_FOREACH(subent, &dir_d->d_subdirs, d_subdirs_link)
+               check_entry();
+       TAILQ_FOREACH(subent, &k_i_info->children, d_subdirs_link)
+               check_entry();
+       if (!found)
+               return -ENOENT;
+       if (count - 1 == desired_file)          /* found the last dir in the list */
                return 0;
        return 1;                                                       /* normal success for readdir */
 }
index 7408b8a..2600914 100644 (file)
@@ -698,6 +698,7 @@ struct inode *get_inode(struct dentry *dentry)
        inode->i_ino = 0;                                       /* set by caller later */
        inode->i_blksize = sb->s_blocksize;
        spinlock_init(&inode->i_lock);
+       kref_get(&sb->s_kref, 1);                       /* could allow the dentry to pin it */
        inode->i_sb = sb;
        inode->i_rdev = 0;                                      /* this has no real meaning yet */
        inode->i_bdev = sb->s_bdev;                     /* storing an uncounted ref */
@@ -960,36 +961,41 @@ ssize_t generic_dir_read(struct file *file, char *u_buf, size_t count,
                          off_t *offset)
 {
        struct kdirent dir_r = {0}, *dirent = &dir_r;
-       unsigned int num_dirents = count / sizeof(struct kdirent);
        int retval = 1;
        size_t amt_copied = 0;
        char *buf_end = u_buf + count;
 
-       if (!count)
-               return 0;
-       if (*offset % sizeof(struct kdirent)) {
-               printk("[kernel] the f_pos for a directory should be dirent-aligned\n");
-               set_errno(EINVAL);
+       if (!S_ISDIR(file->f_dentry->d_inode->i_mode)) {
+               set_errno(ENOTDIR);
                return -1;
        }
-       /* for now, we need to tell readdir which dirent we want */
-       dirent->d_off = *offset / sizeof(struct kdirent);
+       if (!count)
+               return 0;
+       /* start readdir from where it left off: */
+       dirent->d_off = *offset;
        for (; (u_buf < buf_end) && (retval == 1); u_buf += sizeof(struct kdirent)){
                /* TODO: UMEM/KFOP (pin the u_buf in the syscall, ditch the local copy,
                 * get rid of this memcpy and reliance on current, etc).  Might be
-                * tricky with the dirent->d_off */
+                * tricky with the dirent->d_off and trust issues */
                retval = file->f_op->readdir(file, dirent);
-               if (retval < 0)
+               if (retval < 0) {
+                       set_errno(-retval);
                        break;
+               }
+               /* Slight info exposure: could be extra crap after the name in the
+                * dirent (like the name of a deleted file) */
                if (current) {
                        memcpy_to_user(current, u_buf, dirent, sizeof(struct dirent));
                } else {
                        memcpy(u_buf, dirent, sizeof(struct dirent));
                }
                amt_copied += sizeof(struct dirent);
-               dirent->d_off++;
        }
-       *offset += amt_copied;
+       /* Next time read is called, we pick up where we left off */
+       *offset = dirent->d_off;        /* UMEM */
+       /* important to tell them how much they got.  they often keep going til they
+        * get 0 back (in the case of ls).  it's also how much has been read, but it
+        * isn't how much the f_pos has moved (which is opaque to the VFS). */
        return amt_copied;
 }
 
@@ -1739,10 +1745,9 @@ char *do_getcwd(struct fs_struct *fs_env, char **kfree_this, size_t cwd_l)
 static void print_dir(struct dentry *dentry, char *buf, int depth)
 {
        struct dentry *child_d;
-       struct dirent next;
+       struct dirent next = {0};
        struct file *dir;
        int retval;
-       int child_num = 0;
 
        if (!S_ISDIR(dentry->d_inode->i_mode)) {
                warn("Thought this was only directories!!");
@@ -1751,6 +1756,9 @@ static void print_dir(struct dentry *dentry, char *buf, int depth)
        /* Print this dentry */
        printk("%s%s/ nlink: %d\n", buf, dentry->d_name.name,
               dentry->d_inode->i_nlink);
+       if (dentry->d_mount_point) {
+               dentry = dentry->d_mounted_fs->mnt_root;
+       }
        if (depth >= 32)
                return;
        /* Set buffer for our kids */
@@ -1760,9 +1768,12 @@ static void print_dir(struct dentry *dentry, char *buf, int depth)
                panic("Filesystem seems inconsistent - unable to open a dir!");
        /* Process every child, recursing on directories */
        while (1) {
-               next.d_off = child_num++;
                retval = dir->f_op->readdir(dir, &next);
                if (retval >= 0) {
+                       /* Skip .., ., and empty entries */
+                       if (!strcmp("..", next.d_name) || !strcmp(".", next.d_name) ||
+                           next.d_ino == 0)
+                               goto loop_next;
                        /* there is an entry, now get its dentry */
                        child_d = do_lookup(dentry, next.d_name);
                        if (!child_d)
@@ -1793,6 +1804,7 @@ static void print_dir(struct dentry *dentry, char *buf, int depth)
                        }
                        kref_put(&child_d->d_kref);     
                }
+loop_next:
                if (retval <= 0)
                        break;
        }