On Thu, Feb 24, 2005 at 01:33:18AM -0800, Gerrit Huizenga wrote:
> On Mon, 29 Nov 2004 14:15:48 PST, Greg KH wrote:
> > > +/*
> > > + * Address of variable used as flag to indicate a magic file,
> > > + * value unimportant
> > > + */
> > > +int RCFS_IS_MAGIC;
> >
> > Shouldn't this be static?
>
> Nope - used across files.
It shouldn't be.
> > 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?
>
> I believe that these are auto-created file entries which are instantiated
> when a class is created, hence they "magically" appear. They are also
> special in the sense that they are tied to the life of the class, unlike
> other files in the class directories. The MAGIC value is used to help
> distinguish these auto-created entries from other entries in a directory.
> This is a little bit like "." and ".." but specific to the class creation.
That is just wrong.
> > > +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?
>
> I believe they are both exported so resource controllers can create
> class specific directories (including corresponding magic files)
> in the case of _rcfs_mknod, and rcfs_mknod is the exported fs op
> which allows a restricted set of standard user filesystem operations
> within the created directory. So, yes.
Then name the functions sanely. As it is, they are not descriptive of
what they actually do at all.
> > > + __copy_from_user(optbuf, buf, count);
> > > + if (optbuf[count - 1] == '\n')
> > > + optbuf[count - 1] = '\0';
> >
> > Stripping off a single trailing \n character? Why?
>
> I believe this is the "echo value > /rcfs/class/magic_file". If
> there is a newline, it would show up as an extra newline during
> an ls. Of course, Shailabh can correct me if I'm wrong on this one.
So what if a user adds 2 \n to the end of the string. You missed that
one. If you are going to try to protect the names of your files, don't
do it half-hearted...
> > > +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.
>
> Stupid name gone. I didn't grok the need for the #define though?
Use a #define, not a inline function for this, like everyone else in the
kernel does for their container_of macros. Actually inline functions
can't be exported anyway...
> > > +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?
>
> It may be a module, but yes, this should be auto-set in the future when
> CKRM is enabled.
Then fix it? :)
greg k-h
-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click
_______________________________________________
ckrm-tech mailing list
https://lists.sourceforge.net/lists/listinfo/ckrm-tech
