On Thu, Mar 03, 2005 at 12:08:19PM -0800, Gerrit Huizenga wrote:
>
> Hi Chandra, a few comments interpersed... But can you generally kill
> off the C++ comments? They are mixed in quite inconsistently and
> it makes it a little odd to read at times.
fixed
>
> Lots of extra { }'s included. Please strip those out in all 5
> patches.
fixed
>
>
> Should this be on a new line, e.g.:
>
> obj-$(CONFIG_CKRM_RBCE) += rbce/
fixed
>
> > +#define MAGIC_CE_DIR "ce"
> > +#define MAGIC_RULES_DIR "rules"
> > +#define MAGIC_RBCE_INFO "rbce_info"
> > +#define MAGIC_RBCE_STATE "rbce_state"
> > +#define MAGIC_RBCE_TAG "rbce_tag"
> > +
>
> Weren't we going to rename this MAGIC_ to something like
> CONFFILE_ ? And shouldn't these defines be in a header rather than
> the .c file?
fixed
>
> > +
> > + line = (char *)kmalloc(len + 1, GFP_KERNEL);
>
> Cast unnecessary.
fixed
>
> > + len = rc;
> > + }
>
> Too many { }'s - not need to surround a single statement. I think
> there are four extra sets here.
fixed
> > + if (!strcmp(file->f_dentry->d_name.name, MAGIC_RBCE_TAG)) {
> > + return -EPERM;
> > + }
>
> Unnecessary { }'s.
fixed
> > + (for "." entry) */
>
> Can you use the
>
> /*
> * Comment, Line 1
> * Comment, Line 2
> */
>
> Format for multiline comments and /* comment */ for inline or single
> line comments? I just like a little consistency in my own life. ;-)
>
fixed
>
> > +/* SMP-safe */
>
> Why?
fixed
> > + return -EINVAL;
> > + }
>
> Comments, { }'s.
fixed
> > +
> > +// CE allows only the rules directory to be created
>
> Back and forth, C++, C style comments. I like C style. ;-)
fixed
>
> > +int rbce_mkdir(struct inode *dir, struct dentry *dentry, int mode)
> > +{
> > + int retval = -EINVAL;
> > +
> > + struct dentry *pd =
> > + list_entry(dir->i_dentry.next, struct dentry, d_alias);
> > +
> > + // Allow only /rcfs/ce and ce/rules
>
> Allow them for what? Or allow files to only be created in
> this directories? Go for a complete (short is fine) sentence. Comments
> are good but only when they bring enlightenment.
made the comments clear
>
> > +
> > +// CE doesn't allow deletion of directory
>
> Which directory? Any directory?
yes, any directory.. as of now there is only one directory rules, which is
automatically created.
>
> > +
> > +/******************************* Magic files ********************/
>
> Magic, hmm. Is this a sufficiently advanced technology?
:) fixed
>
> > + * for cleanliness.
>
> Okay - I don't get this. Why are all these messages in the kernel?
> Is this an in-kernel help file? Ugh. I don't think I like this. Does
> this really have to be here?
as discussed, removed this and added it in the documentation directory
> > @@ -0,0 +1,68 @@
> > +/*
> > + * rbce internal header file.
>
> Are there some headers that are not internal? Are they supposed to
> be used by user level applications? Or by whom? Can you call out those
> header files and ensure that they have *only* definitions/declarations
> needed by the scope in which they are to be used? And then this
> "internal" name could be made into something more descriptive.
I meant to mean it as "internal to the rbce module". It is being as
filenames in some other parts of the kernel too....
Filename has not been changed. Suggestions for new name welcome.
>
> > +#ifndef _RBCE_INTERNAL_H
> > +#define _RBCE_INTERNAL_H
> > +
> > +#define DEBUG
>
> Do you still need this in here?
gone
>
> > +// Rule handling data structures.
> > +struct named_obj_hdr {
> > + struct list_head link;
> > + int referenced;
> > + char *name;
> > +};
>
> Okay, the comment isn't very clear and the structure name is
> even less clear. Is this an rbce_rule ?
made comment clear... it is used as a header for both rule and class data
structure that are internal to rbce.
>
> > +extern int rbce_set_tasktag(int, char *);
> > +extern int rbce_rename_rule(const char *, const char *);
>
> Why are these all "char *"? Shouldn't they be passing around
> rule structures or something? No, I haven't read ahead yet. ;)
no, they are char *, these are interfaces from the filesystem to the main
functionality.
>
> > +
> > + printk(KERN_INFO "<1>\nInstalling \'%s\' module\n", modname);
>
> Isn't the KERN_INFO and <1> redundent?
fixed
>
> > + rcfs_unregister_engine(&rbce_rcfs_ecbs);
> > + printk(KERN_ERR "<1>%s: error installing rc=%d line=%d\n",
>
> Why the <1> again?
>
> And this seems inconsistent and/or wrong - you have a return higher
> in the function and one goto. Only one way to get here, so __LINE__
> can only have one value...
fixed
>
> > + __FUNCTION__, rc, line);
> > + return rc;
> > +}
> > +
> > +void exit_rbce(void)
> > +{
> > + printk(KERN_INFO "<1>Removing \'%s\' module\n", modname);
>
> I'm still thinking KERN_INFO and <1> are redundent. Prove me
> wrong if you want...
fixed
>
> gerrit
--
----------------------------------------------------------------------
Chandra Seetharaman | Be careful what you choose....
- [EMAIL PROTECTED] | .......you may get it.
----------------------------------------------------------------------
-------------------------------------------------------
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
