Hi Michael,
I have included some review comments below.
Pekka
On 4/21/06, Michael Holzheu <[email protected]> wrote:
> diff -urpN linux-2.6.16/fs/hypfs/hypfs.h linux-2.6.16-hypfs/fs/hypfs/hypfs.h
> --- linux-2.6.16/fs/hypfs/hypfs.h 1970-01-01 01:00:00.000000000 +0100
> +++ linux-2.6.16-hypfs/fs/hypfs/hypfs.h 2006-04-21 12:56:58.000000000 +0200
> +static void inline remove_trailing_blanks(char *string)
> +{
> + char *ptr;
> + for (ptr = string + strlen(string) - 1; ptr > string; ptr--) {
> + if (*ptr == ' ')
> + *ptr = 0;
> + else
> + break;
> + }
> +}
Please consider moving this to lib/string.c and perhaps renaming it to
strstrip().
> diff -urpN linux-2.6.16/fs/hypfs/hypfs_diag.c linux-2.6.16-hypfs/fs/hypfs/hypfs_diag.c
> --- linux-2.6.16/fs/hypfs/hypfs_diag.c 1970-01-01 01:00:00.000000000 +0100
> +++ linux-2.6.16-hypfs/fs/hypfs/hypfs_diag.c 2006-04-21 12:56:58.000000000 +0200
> +/* diag 204 subcodes */
> +typedef enum {
> + SUBC_STIB4 = 4,
> + SUBC_RSI = 5,
> + SUBC_STIB6 = 6,
> + SUBC_STIB7 = 7
> +} diag204_subc_t;
> +
> +/* The two available diag 204 data formats */
> +typedef enum {
> + INFO_SIMPLE = 0,
> + INFO_EXT = 0x00010000
> +} diag204_info_t;
Please kill the typedefs.
> +/* Time information block */
> +
> +struct info_blk_hdr {
> + __u8 npar;
> + __u8 flags;
> + __u16 tslice;
> + __u16 phys_cpus;
> + __u16 this_part;
> + __u64 curtod;
> +} __attribute__ ((packed));
> +
> +struct x_info_blk_hdr {
> + __u8 npar;
> + __u8 flags;
> + __u16 tslice;
> + __u16 phys_cpus;
> + __u16 this_part;
> + __u64 curtod1;
> + __u64 curtod2;
> + char reserved[40];
> +} __attribute__ ((packed));
Couldn't you use endianess annotated types for these?
> +/* Partition header */
> +
> +struct part_hdr {
> + __u8 pn;
> + __u8 cpus;
> + char reserved[6];
> + char part_name[LPAR_NAME_LEN];
> +} __attribute__ ((packed));
> +
> +struct x_part_hdr {
> + __u8 pn;
> + __u8 cpus;
> + __u8 rcpus;
> + __u8 pflag;
> + __u32 mlu;
> + char part_name[LPAR_NAME_LEN];
> + char lpc_name[8];
> + char os_name[8];
> + __u64 online_cs;
> + __u64 online_es;
> + __u8 upid;
> + char reserved1[3];
> + __u32 group_mlu;
> + char group_name[8];
> + char reserved2[32];
> +} __attribute__ ((packed));
Same here.
> +
> +static inline int part_hdr__size(diag204_info_t type)
Please drop the extra underscore from function name.
> +static inline __u8 part_hdr__cpus(diag204_info_t type, void *hdr)
Ditto. Appears in various other places as well.
> +struct cpu_info {
> + __u16 cpu_addr;
> + char reserved1[2];
> + __u8 ctidx;
> + __u8 cflag;
> + __u16 weight;
> + __u64 acc_time;
> + __u64 lp_time;
> +} __attribute__ ((packed));
> +
> +struct x_cpu_info {
> + __u16 cpu_addr;
> + char reserved1[2];
> + __u8 ctidx;
> + __u8 cflag;
> + __u16 weight;
> + __u64 acc_time;
> + __u64 lp_time;
> + __u16 min_weight;
> + __u16 cur_weight;
> + __u16 max_weight;
> + char reseved2[2];
> + __u64 online_time;
> + __u64 wait_time;
> + __u32 pma_weight;
> + __u32 polar_weight;
> + char reserved3[40];
> +} __attribute__ ((packed));
Endianess annotated types?
> +/* Physical header */
> +
> +struct phys_hdr {
> + char reserved1[1];
> + __u8 cpus;
> + char reserved2[6];
> + char mgm_name[8];
> +} __attribute__ ((packed));
> +
> +struct x_phys_hdr {
> + char reserved1[1];
> + __u8 cpus;
> + char reserved2[6];
> + char mgm_name[8];
> + char reserved3[80];
> +} __attribute__ ((packed));
Here.
> +/* Physical CPU info block */
> +
> +struct phys_cpu {
> + __u16 cpu_addr;
> + char reserved1[2];
> + __u8 ctidx;
> + char reserved2[3];
> + __u64 mgm_time;
> + char reserved3[8];
> +} __attribute__ ((packed));
> +
> +struct x_phys_cpu {
> + __u16 cpu_addr;
> + char reserved1[2];
> + __u8 ctidx;
> + char reserved2[3];
> + __u64 mgm_time;
> + char reserved3[80];
> +} __attribute__ ((packed));
Here.
> +static inline int diag204(unsigned long subcode, unsigned long size, void *addr)
> +{
> + register unsigned long _subcode asm("0") = subcode;
> + register unsigned long _size asm("1") = size;
> +
> + asm volatile (" diag %2,%0,0x204\n"
> + "0: \n" ".section __ex_table,\"a\"\n"
> +#ifndef __s390x__
> + " .align 4\n"
> + " .long 0b,0b\n"
> +#else
> + " .align 8\n"
> + " .quad 0b,0b\n"
> +#endif
> + ".previous":"+d" (_subcode), "+d"(_size)
> + :"d"(addr)
> + :"memory");
Please note that the above is a big clue for this fs to go into arch/s390/.
> +static int diag204_probe(void)
> +{
> + void *buf;
> + int pages, rc;
> +
> + pages = diag204(SUBC_RSI | INFO_EXT, 0, 0);
> + if (pages > 0) {
> + if (!(buf = kmalloc(pages * PAGE_SIZE, GFP_KERNEL))) {
Please move the assignment out of the if expression for readability.
> + rc = -ENOMEM;
> + goto err_out;
> + }
> + if (diag204(SUBC_STIB7 | INFO_EXT, pages, buf) >= 0) {
> + diag204_store_sc = SUBC_STIB7;
> + diag204_info_type = INFO_EXT;
> + goto out;
> + }
> + if (diag204(SUBC_STIB6 | INFO_EXT, pages, buf) >= 0) {
> + diag204_store_sc = SUBC_STIB7;
> + diag204_info_type = INFO_EXT;
> + goto out;
> + }
> + kfree(buf);
> + }
> + if (!(buf = kmalloc(PAGE_SIZE, GFP_KERNEL))) {
Same here.
> + rc = -ENOMEM;
> + goto err_out;
> + }
> + if (diag204(SUBC_STIB4 | INFO_SIMPLE, pages, buf) >= 0) {
> + diag204_store_sc = SUBC_STIB4;
> + diag204_info_type = INFO_SIMPLE;
> + goto out;
> + } else {
> + rc = -ENOSYS;
> + goto err_out;
> + }
> + out:
> + kfree(buf);
> + return 0;
> + err_out:
> + kfree(buf);
> + return rc;
Please drop the duplicate error path.
> +static void *diag204_store(void)
> +{
> + void *buf;
> + int pages;
> +
> + if (diag204_store_sc == SUBC_STIB4)
> + pages = 1;
> + else
> + pages = diag204(SUBC_RSI | diag204_info_type, 0, 0);
> +
> + if (pages < 0)
> + return ERR_PTR(-ENOSYS);
> +
> + if (!(buf = kmalloc(pages * PAGE_SIZE, GFP_KERNEL)))
And here.
> diff -urpN linux-2.6.16/fs/hypfs/inode.c linux-2.6.16-hypfs/fs/hypfs/inode.c
> --- linux-2.6.16/fs/hypfs/inode.c 1970-01-01 01:00:00.000000000 +0100
> +++ linux-2.6.16-hypfs/fs/hypfs/inode.c 2006-04-21 12:56:58.000000000 +0200
> +static DECLARE_MUTEX(hypfs_lock); // XXX DEFINE_MUTEX in 2.6.16 !
Yes, please!
> +
> +/* start of list of all dentries, which have to be deleted on update */
> +static struct dentry *hypfs_last_dentry;
> +
> +static void hypfs_update_update(void)
> +{
> + last_update_time = get_seconds();
> + snprintf((char *)update_file_dentry->d_inode->u.generic_ip,
> + UPDATE_DATA_SIZE, "%ld\n", last_update_time);
> + update_file_dentry->d_inode->i_size =
> + strlen((char *)update_file_dentry->d_inode->u.generic_ip);
> + update_file_dentry->d_inode->i_atime =
> + update_file_dentry->d_inode->i_mtime =
> + update_file_dentry->d_inode->i_ctime = CURRENT_TIME;
Please introduce local variable for update_file_dentry->d_inode for
readability. Perhaps for generic_ip as well.
> +}
> +
> +/* directory tree removal functions */
> +
> +static void hypfs_add_dentry(struct dentry *dentry)
> +{
> + dentry->d_fsdata = hypfs_last_dentry;
> + hypfs_last_dentry = dentry;
> +}
> +
> +static void hypfs_delete_tree(struct dentry *root)
> +{
> + while (hypfs_last_dentry) {
> + struct dentry *parent, *next_dentry;
> +
> + parent = hypfs_last_dentry->d_parent;
> + if (S_ISDIR(hypfs_last_dentry->d_inode->i_mode))
> + simple_rmdir(parent->d_inode, hypfs_last_dentry);
> + else
> + simple_unlink(parent->d_inode, hypfs_last_dentry);
> + d_delete(hypfs_last_dentry);
> + next_dentry = hypfs_last_dentry->d_fsdata;
> + dput(hypfs_last_dentry);
> + hypfs_last_dentry = next_dentry;
> + }
> +}
> +
> +static struct inode *hypfs_make_inode(struct super_block *sb, int mode)
> +{
> + struct inode *ret = new_inode(sb);
> +
> + if (ret) {
> + ret->i_mode = mode;
> + ret->i_uid = ((struct hypfs_sb_info *)sb->s_fs_info)->uid;
> + ret->i_gid = ((struct hypfs_sb_info *)sb->s_fs_info)->gid;
Reduce casting by introducing a local variable for hypfs_sb_info.
> + ret->i_blksize = PAGE_CACHE_SIZE;
> + ret->i_blocks = 0;
> + ret->i_atime = ret->i_mtime = ret->i_ctime = CURRENT_TIME;
> + if (mode & S_IFDIR)
> + ret->i_nlink = 2;
> + else
> + ret->i_nlink = 1;
> + }
> + return ret;
> +}
> +
> +static void hypfs_drop_inode(struct inode *inode)
> +{
> + kfree((void *)inode->u.generic_ip);
Please drop the redundant cast. Consider moving this to inode_ops->clear_inode.
> + generic_delete_inode(inode);
> +}
> +static int hypfs_parse_options(char *options)
> +{
> + char *str;
> + substring_t args[MAX_OPT_ARGS];
> +
> + if (!options)
> + return 0;
> + while ((str = strsep(&options, ",")) != NULL) {
> + int token, option;
> + if (!*str)
> + continue;
> + token = match_token(str, hypfs_tokens, args);
> + switch (token) {
> + case opt_uid:
> + if (match_int(&args[0], &option))
> + return -EINVAL;
> + ((struct hypfs_sb_info *)hypfs_sblk->s_fs_info)->uid
> + = option;
> + break;
> + case opt_gid:
> + if (match_int(&args[0], &option))
> + return -EINVAL;
> + ((struct hypfs_sb_info *)hypfs_sblk->s_fs_info)->gid
> + = option;
Please reduce casting by introducing a local variable for hypfs_sb_info.
> +static int hypfs_fill_super(struct super_block *sb, void *data, int silent)
> +{
> + struct inode *root_inode;
> + int rc = 0;
> + struct hypfs_sb_info *sbi;
> +
> + sbi = kmalloc(sizeof(struct hypfs_sb_info), GFP_KERNEL);
> + if (!sbi)
> + return -ENOMEM;
> + sbi->uid = current->uid;
> + sbi->gid = current->gid;
> + sb->s_fs_info = sbi;
> + sb->s_blocksize = PAGE_CACHE_SIZE;
> + sb->s_blocksize_bits = PAGE_CACHE_SHIFT;
> + sb->s_magic = HYPFS_MAGIC;
> + sb->s_op = &hypfs_s_ops;
> + if (hypfs_parse_options(data)) {
> + rc = -EINVAL;
> + goto err_alloc;
> + }
> + root_inode = hypfs_make_inode(sb, S_IFDIR | 0755);
> + if (!root_inode) {
> + rc = -ENOMEM;
> + goto err_alloc;
> + }
> + root_inode->i_op = &simple_dir_inode_operations;
> + root_inode->i_fop = &simple_dir_operations;
> + sb->s_root = d_alloc_root(root_inode);
> + if (!sb->s_root) {
> + rc = -ENOMEM;
> + goto err_inode;
> + }
> + hypfs_sblk = sb;
> + rc = diag_create_files(hypfs_sblk, hypfs_sblk->s_root);
> + if (rc)
> + goto err_tree;
> + update_file_dentry = hypfs_create_update_file(hypfs_sblk,
> + hypfs_sblk->s_root);
> + if (IS_ERR(update_file_dentry)) {
> + rc = PTR_ERR(update_file_dentry);
> + goto err_tree;
> + }
> + hypfs_update_update();
> + return 0;
> +
> + err_tree:
> + hypfs_delete_tree(hypfs_sblk->s_root);
> + dput(hypfs_sblk->s_root);
> + err_inode:
> + hypfs_drop_inode(root_inode);
You should use iput() here.
> + err_alloc:
> + kfree(sbi);
> + return rc;
> +}
> +static void hypfs_kill_super(struct super_block *sb)
> +{
> + kfree(sb->s_fs_info);
Please use super_operations->put_super instead for freeing s_fs_info.
> + kill_litter_super(sb);
> +/*
> + * init and exit
> + * *************
> + */
Consider dropping the above useless comment.
> +
> +static decl_subsys(hypervisor, NULL, NULL);
> +
> +static int __init hypfs_init(void)
> +{
> + int rc;
> +
> + if (MACHINE_IS_VM) {
> + return -ENODATA;
> + }
> + if (diag_init()) {
> + printk(KERN_ERR "hypfs: diag init failed.\n");
> + return -ENODATA;
> + }
> + rc = subsystem_register(&hypervisor_subsys);
> + if (rc) {
> + diag_exit();
> + return rc;
> + }
> + rc = register_filesystem(&hypfs_type);
> + if (rc) {
> + subsystem_unregister(&hypervisor_subsys);
> + diag_exit();
> + return rc;
Please use gotos for error handling here to reduce duplication.
-
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]
