Re: [PATCH 6/13: eCryptfs] Superblock operations

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Phillip,

Some comments below.

On 5/4/06, Phillip Hellewell <[email protected]> wrote:
+kmem_cache_t *ecryptfs_inode_info_cache;

Please use struct kmem_cache instead of the typedef.

+static struct inode *ecryptfs_alloc_inode(struct super_block *sb) {

Formatting

+       struct ecryptfs_inode_info *ecryptfs_inode = NULL;

Redundant initialization

+       struct inode *inode = NULL;
+
+       ecryptfs_printk(KERN_DEBUG, "Enter; sb = [%p]\n", sb);
+       ecryptfs_inode = kmem_cache_alloc(ecryptfs_inode_info_cache,
+                                         SLAB_KERNEL);
+       if (unlikely(!ecryptfs_inode)) {
+               ecryptfs_printk(KERN_WARNING,
+                               "Failed to allocate new inode\n");
+               goto out;

No need to log it, just return NULL here

+       }
+       ecryptfs_init_crypt_stat(&(ecryptfs_inode->crypt_stat));
+       inode = &(ecryptfs_inode->vfs_inode);

Bogus parenthesis, twice. Inode is a redundant local variable, btw.

+out:
+       ecryptfs_printk(KERN_DEBUG, "Exit; inode = [%p]\n", inode);
+       return inode;
+}
+
+/**
+ * This is used during the final destruction of the inode.
+ * All allocation of memory related to the inode, including allocated
+ * memory in the crypt_stat struct, will be released here.
+ * There should be no chance that this deallocation will be missed.
+ */
+static void ecryptfs_destroy_inode(struct inode *inode) {

Formatting

+       struct ecryptfs_crypt_stat *crypt_stat;
+
+       ecryptfs_printk(KERN_DEBUG, "Enter; inode = [%p]\n", inode);
+       crypt_stat = &(ECRYPTFS_INODE_TO_PRIVATE(inode))->crypt_stat;
+       ecryptfs_destruct_crypt_stat(crypt_stat);
+       kmem_cache_free(ecryptfs_inode_info_cache,
+                       ECRYPTFS_INODE_TO_PRIVATE(inode));

Better to introduce a local variable for CRYPTFS_INODE_TO_PRIVATE.
More readable and smaller kernel text that way.

+       ecryptfs_printk(KERN_DEBUG, "Exit\n");
+}
+
+/**
+ * Set up the ecryptfs inode.
+ */
+static void ecryptfs_read_inode(struct inode *inode)
+{
+       ecryptfs_printk(KERN_DEBUG, "Enter; inode = [%p]\n", inode);
+       /* This is where we setup the self-reference in the vfs_inode's
+        * u.generic_ip. That way we don't have to walk the list again. */
+       ECRYPTFS_INODE_TO_PRIVATE_SM(inode) =
+               list_entry(inode, struct ecryptfs_inode_info, vfs_inode);
+       ECRYPTFS_INODE_TO_LOWER(inode) = NULL;

Hmm, ugly, please make the setters explicit instead.

+       inode->i_version++;
+       inode->i_op = &ecryptfs_main_iops;
+       inode->i_fop = &ecryptfs_main_fops;
+       inode->i_mapping->a_ops = &ecryptfs_aops;
+       ecryptfs_printk(KERN_DEBUG, "Exit\n");
+}
+
+
+/**
+ * This is called through iput_final().
+ * This is function will replace generic_drop_inode. The end result of which
+ * is we are skipping the check in inode->i_nlink, which we do not use.
+ */
+static void ecryptfs_drop_inode(struct inode *inode) {

Formatting

+       generic_delete_inode(inode);
+}
+
+/**
+ * Final actions when unmounting a file system.
+ * This will handle deallocation and release of our private data.
+ */
+static void ecryptfs_put_super(struct super_block *sb)
+{
+       struct ecryptfs_sb_info *sb_info = ECRYPTFS_SUPERBLOCK_TO_PRIVATE(sb);

You probably want to rename ECRYPTFS_SUPERBLOCK_TO_PRIVATE to
ecryptfs_sb or similar.

+
+       ecryptfs_printk(KERN_DEBUG, "Enter\n");
+       mntput(sb_info->lower_mnt);
+       key_put(sb_info->mount_crypt_stat.global_auth_tok_key);
+       kmem_cache_free(ecryptfs_sb_info_cache, sb_info);
+       ECRYPTFS_SUPERBLOCK_TO_PRIVATE_SM(sb) = NULL;
+       ecryptfs_printk(KERN_DEBUG, "Exit\n");
+}
+
+/**
+ * Get the filesystem statistics. Currently, we let this pass right through
+ * to the lower filesystem and take no action ourselves.
+ */
+static int ecryptfs_statfs(struct super_block *sb, struct kstatfs *buf)
+{
+       int rc = 0;

Redundant initialization

+
+       ecryptfs_printk(KERN_DEBUG, "Enter\n");
+       rc = vfs_statfs(ECRYPTFS_SUPERBLOCK_TO_LOWER(sb), buf);
+       ecryptfs_printk(KERN_DEBUG, "Exit; rc = [%d]\n", rc);
+       return rc;
+}
+
+/**
+ * Called to ask filesystem to change mount options. Not implemented;
+ * returns -ENOSYS every time.
+ */
+static int ecryptfs_remount_fs(struct super_block *sb, int *flags, char *data)
+{
+       ecryptfs_printk(KERN_DEBUG, "Enter\n");
+       ecryptfs_printk(KERN_DEBUG, "Exit\n");
+       return -ENOSYS;
+}

Any reason you want to keep this around?

+
+/**
+ * Called by iput() when the inode reference count reached zero
+ * and the inode is not hashed anywhere.  Used to clear anything
+ * that needs to be, before the inode is completely destroyed and put
+ * on the inode free list. We use this to drop out reference to the
+ * lower inode.
+ */
+static void ecryptfs_clear_inode(struct inode *inode)
+{
+       ecryptfs_printk(KERN_DEBUG, "Enter; inode = [%p]; i_ino = [0x%.16x]\n",
+                       inode, inode->i_ino);
+       iput(ECRYPTFS_INODE_TO_LOWER(inode));
+       ecryptfs_printk(KERN_DEBUG, "Exit\n");
+}
+
+/**
+ * Called in do_umount() if the MNT_FORCE flag was used and this
+ * function is defined.  See comment in linux/fs/super.c:do_umount().
+ * Used only in nfs, to kill any pending RPC tasks, so that subsequent
+ * code can actually succeed and won't leave tasks that need handling.
+ */
+static void ecryptfs_umount_begin(struct vfsmount *vfsmnt, int flags)
+{
+       struct vfsmount *lower_mnt;
+       struct super_block *lower_sb;
+
+       ecryptfs_printk(KERN_DEBUG, "Enter\n");
+       lower_mnt = ECRYPTFS_SUPERBLOCK_TO_PRIVATE(vfsmnt->mnt_sb)->lower_mnt;
+       lower_sb = lower_mnt->mnt_sb;
+       if (lower_sb->s_op->umount_begin)
+               lower_sb->s_op->umount_begin(lower_mnt, flags);
+       ecryptfs_printk(KERN_DEBUG, "Exit\n");
+}
+
+/**
+ * Prints the directory we are currently mounted over
+ *
+ * @return Zero on success; non-zero otherwise
+ */
+static int ecryptfs_show_options(struct seq_file *m, struct vfsmount *mnt)
+{
+       struct super_block *sb = mnt->mnt_sb;
+       int rc = 0;
+       char *tmp = NULL;
+       char *path;
+
+       ecryptfs_printk(KERN_DEBUG, "Enter\n");
+       tmp = (char *)__get_free_page(GFP_KERNEL);
+       if (!tmp) {
+               rc = -ENOMEM;
+               goto out;
+       }
+       path = d_path(ECRYPTFS_DENTRY_TO_LOWER(sb->s_root),
+                     ECRYPTFS_SUPERBLOCK_TO_PRIVATE(sb)->lower_mnt, tmp,
+                     PAGE_SIZE);

Use of local variables probably would clean that up

+       seq_printf(m, ",dir=%s", path);
+       free_page((unsigned long)tmp);
+out:
+       ecryptfs_printk(KERN_DEBUG, "Exit; rc = [%d]\n", rc);
+       return rc;
+}
+
+struct super_operations ecryptfs_sops = {
+       .alloc_inode = ecryptfs_alloc_inode,
+       .destroy_inode = ecryptfs_destroy_inode,
+       .read_inode = ecryptfs_read_inode,
+       .drop_inode = ecryptfs_drop_inode,
+       .put_super = ecryptfs_put_super,
+       .statfs = ecryptfs_statfs,
+       .remount_fs = ecryptfs_remount_fs,
+       .clear_inode = ecryptfs_clear_inode,
+       .umount_begin = ecryptfs_umount_begin,
+       .show_options = ecryptfs_show_options
+};
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Stuff]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]     [Linux Resources]
  Powered by Linux