lkml.org 
[lkml]   [2004]   [Nov]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] CKRM: 4/10 CKRM: Full rcfs support
On Mon, Nov 29, 2004 at 10:48:24AM -0800, Gerrit Huizenga wrote:
> +#include <linux/module.h>
> +#include <linux/list.h>
> +#include <linux/fs.h>
> +#include <linux/namei.h>
> +#include <linux/namespace.h>
> +#include <linux/dcache.h>
> +#include <linux/seq_file.h>
> +#include <linux/pagemap.h>
> +#include <linux/highmem.h>
> +#include <linux/init.h>
> +#include <linux/string.h>
> +#include <linux/smp_lock.h>
> +#include <linux/backing-dev.h>
> +#include <linux/parser.h>
> +#include <asm/uaccess.h>
> +
> +#include <linux/rcfs.h>

asm last please.

> +/*
> + * Address of variable used as flag to indicate a magic file,
> + * value unimportant
> + */
> +int RCFS_IS_MAGIC;

Shouldn't this be static?

And what is a "magic" file used for? I see where you set something to
point to this, but no where do you check for it. What's the use of it?

> +int _rcfs_mknod(struct inode *dir, struct dentry *dentry, int mode, dev_t dev)
> +{
> + struct inode *inode;
> + int error = -EPERM;
> +
> + if (dentry->d_inode)
> + return -EEXIST;
> + inode = rcfs_get_inode(dir->i_sb, mode, dev);
> + if (inode) {
> + if (dir->i_mode & S_ISGID) {
> + inode->i_gid = dir->i_gid;
> + if (S_ISDIR(mode))
> + inode->i_mode |= S_ISGID;
> + }
> + d_instantiate(dentry, inode);
> + dget(dentry);
> + error = 0;
> + }
> + return error;
> +}
> +
> +EXPORT_SYMBOL_GPL(_rcfs_mknod);
> +
> +int rcfs_mknod(struct inode *dir, struct dentry *dentry, int mode, dev_t dev)
> +{
> + /* User can only create directories, not files */
> + if ((mode & S_IFMT) != S_IFDIR)
> + return -EINVAL;
> +
> + return dir->i_op->mkdir(dir, dentry, mode);
> +}
> +
> +EXPORT_SYMBOL_GPL(rcfs_mknod);

Why 2 mknod functions? Do they both really need to be exported?

> +
> +#define MAGIC_SHOW(FUNC) \
> +static int \

You mix tabs and spaces in your #defines in this file, please just use
tabs properly.

> +static ssize_t
> +target_reclassify_write(struct file *file, const char __user * buf,
> + size_t count, loff_t * ppos, int manual)
> +{
> + struct rcfs_inode_info *ri = RCFS_I(file->f_dentry->d_inode);
> + char *optbuf;
> + int rc = -EINVAL;
> + ckrm_classtype_t *clstype;
> +
> + if ((ssize_t) count < 0 || (ssize_t) count > TARGET_MAX_INPUT_SIZE)
> + return -EINVAL;

But count is an unsigned variable, right? How could it ever be
negative?

> + if (!access_ok(VERIFY_READ, buf, count))
> + return -EFAULT;
> + down(&(ri->vfs_inode.i_sem));
> + optbuf = kmalloc(TARGET_MAX_INPUT_SIZE, GFP_KERNEL);

kmalloc with a lock held? Is that a good idea?

You also don't check the return value of kmalloc, that's a bad idea.

> + __copy_from_user(optbuf, buf, count);
> + if (optbuf[count - 1] == '\n')
> + optbuf[count - 1] = '\0';

Stripping off a single trailing \n character? Why?

> +inline struct rcfs_inode_info *RCFS_I(struct inode *inode)
> +{
> + return container_of(inode, struct rcfs_inode_info, vfs_inode);
> +}
> +
> +EXPORT_SYMBOL_GPL(RCFS_I);

This should be named something sane, and just use a #define for it like
most other container_of() users.

> +void rcfs_destroy_inodecache(void)
> +{
> + printk(KERN_WARNING "destroy inodecache was called\n");

Do you really want to print this out in "production" code?

> + if (kmem_cache_destroy(rcfs_inode_cachep))
> + printk(KERN_INFO
> + "rcfs_inode_cache: not all structures were freed\n");

Shouldn't this really be INFO level? What is a user going to do with
this information?

> +config RCFS_FS
> + tristate "Resource Class File System (User API)"
> + depends on CKRM
> + help
> + RCFS is the filesystem API for CKRM. This separate configuration
> + option is provided only for debugging and will eventually disappear
> + since rcfs will be automounted whenever CKRM is configured.
> +
> + Say N if unsure, Y if you've enabled CKRM, M to debug rcfs
> + initialization.
> +

So is this option going to stay around, or should it always be enabled
if CKRM is enabled? Why not just do that for the user?

thanks,

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

\
 
 \ /
  Last update: 2005-03-22 14:08    [W:0.071 / U:0.104 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site