Hi Andrew!
Thanks for the review!
Andrew Morton <[email protected]> wrote on 04/28/2006 11:56:21 AM:
> Michael Holzheu <[email protected]> wrote:
[snip]
>
> > +config HYPFS_FS
> > + bool "s390 hypervisor file system support"
> > + default y
> > + help
> > + This is a virtual file system intended to provide accounting
> > + information in a s390 hypervisor environment.
> > +
>
> "in an s390"?
Yes!
> > +static inline int diag204(unsigned long subcode, unsigned long
> size, void *addr)
>
> There's a lot of inlining going on. Is it excessive?
I will remove the diag inlines. The code is not performance critical
anyway, since diag 204 itself is soo slow. But I would like to keep
the inlines for the getter functions.
> > ...
> >
> > +static int diag204_probe(void)
> > +{
> > + void *buf;
> > + int pages, rc;
> > +
> > + pages = diag204(SUBC_RSI | INFO_EXT, 0, 0);
> > + if (pages > 0) {
>
> In other places, you return an error if (pages < 0), but not here.
I make an additional comment to document the code
here!
... and you are right! In diag204_store() we should check
not for pages < 0, but for pages <= 0! The other spots
are ok.
[snip]
> > + }
> > + out:
> > + rc = 0;
> > + err_out:
> > + kfree(buf);
> > + return rc;
> > +}
>
> The indenting of the labels is a bit unconventional.
Sorry, that was lindent...
> > +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);
> > +
> > + buf = kmalloc(pages * PAGE_SIZE, GFP_KERNEL);
> > + if (!buf)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + if (diag204(diag204_store_sc | diag204_info_type, pages, buf) < 0)
{
> > + kfree(buf);
> > + return ERR_PTR(-ENOSYS);
> > + }
> > + return buf;
> > +}
>
> One wonders if
>
> pages = diag204(...);
> return kmalloc(pages * PAGE_SIZE, gfp_flags);
>
> would be a useful helper function.
Good idea! I implemented a diag204_get_buffer() function, but
I am not sure, if it is better now. At least it is not worse!
> > + asm volatile(" diag %0,%1,0x224\n"
> > + : :"d" (0), "d"(ptr) : "memory");
> > +}
> > +
> > +static inline int diag224_get_name_table(void)
> > +{
> > + /* memory must be below 2GB */
> > + diag224_cpu_names = kmalloc(PAGE_SIZE, GFP_KERNEL | GFP_DMA);
> > + if (!diag224_cpu_names)
> > + return -ENOMEM;
> > + diag224(diag224_cpu_names);
> > + EBCASC(diag224_cpu_names + 16, (*diag224_cpu_names + 1) * 16);
> > + return 0;
> > +}
> > +
> > +static inline void diag224_delete_name_table(void)
> > +{
> > + kfree(diag224_cpu_names);
> > +}
> > +
> > +static int diag224_idx2name(int index, char *name)
> > +{
> > + memcpy(name, diag224_cpu_names + ((index + 1) * 16), 16);
> > + name[16] = 0;
>
> Should this be "15"? I guess not...
No bug, our strings here have 16 characters and are not
0 terminated.
> > + strstrip(name);
> > + return 0;
> > +}
> > +
> > +int diag_init(void)
> > +{
> > + int rc;
> > +
> > + if (diag204_probe()) {
> > + printk(KERN_ERR "hypfs: diag 204 not working.");
> > + return -ENODATA;
> > + }
> > + rc = diag224_get_name_table();
> > + if (rc) {
> > + diag224_delete_name_table();
> > + printk(KERN_ERR "hypfs: could not get name table.\n");
> > + }
> > + return rc;
> > +}
>
> This can be __init
Yes!
> > +void diag_exit(void)
> > +{
> > + diag224_delete_name_table();
> > +}
>
> And __exit.
Yes!
> > +int diag_create_files(struct super_block *sb, struct dentry *root)
> > +{
>
> Perhaps as a globally-visible function this should be called
> hypfs_diag_create_files(). Ditto diag_init() and diag_exit().
Ok, I renamed the functions.
> > + struct dentry *systems_dir, *hyp_dir;
> > + void *time_hdr, *part_hdr;
> > + int i;
> > + void *buffer, *rc;
> > +
> > + buffer = diag204_store();
> > + if (IS_ERR(buffer))
> > + return PTR_ERR(buffer);
> > +
> > + systems_dir = hypfs_mkdir(sb, root, "systems");
> > + if (IS_ERR(systems_dir))
> > + return PTR_ERR(systems_dir);
> > + time_hdr = (struct x_info_blk_hdr *)buffer;
> > + part_hdr = time_hdr + info_blk_hdr__size(diag204_info_type);
> > + for (i = 0; i < info_blk_hdr__npar(diag204_info_type, time_hdr);
i++) {
> > + part_hdr = hypfs_create_lpar_files(sb, systems_dir, part_hdr);
> > + if (IS_ERR(part_hdr))
> > + return PTR_ERR(part_hdr);
> > + }
> > + if (info_blk_hdr__flags(diag204_info_type, time_hdr) &
LPAR_PHYS_FLG) {
> > + void *rc;
> > + rc = hypfs_create_phys_files(sb, root, part_hdr);
> > + if (IS_ERR(rc))
> > + return PTR_ERR(rc);
> > + }
> > + hyp_dir = hypfs_mkdir(sb, root, "hyp");
> > + if (IS_ERR(hyp_dir))
> > + return PTR_ERR(hyp_dir);
> > + rc = hypfs_create_str(sb, hyp_dir, "type", "LPAR Hypervisor");
> > + if (IS_ERR(rc))
> > + return PTR_ERR(rc);
> > + kfree(buffer);
> > + return 0;
> > +}
>
> This leaks `buffer' on the error paths, yes?
Right! I fixed this!
> > +/*
> > + * fs/hypfs/inode.c
> > + * Hypervisor filesystem for Linux on s390.
> > + *
> > + * Copyright (C) IBM Corp. 2006
> > + * Author(s): Michael Holzheu <[email protected]>
> > + */
> > +
> > +#include <linux/types.h>
> > +#include <linux/errno.h>
> > +#include <linux/fs.h>
> > +#include <linux/namei.h>
> > +#include <linux/vfs.h>
> > +#include <linux/pagemap.h>
> > +#include <linux/gfp.h>
> > +#include <linux/time.h>
> > +#include <linux/parser.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/module.h>
> > +#include <asm/ebcdic.h>
> > +#include "hypfs.h"
> > +#include "hypfs_diag.h"
> > +
> > +#define HYPFS_MAGIC 0x687970 /* ASCII 'hyp' */
> > +#define TMP_SIZE 64 /* size of temporary buffers */
> > +
> > +static struct dentry *hypfs_create_update_file(struct super_block *sb,
> > + struct dentry *dir);
> > +
> > +struct hypfs_sb_info {
> > + int uid; /* uid used for files and dirs */
> > + int gid; /* gid used for files and dirs */
> > +};
>
> uid_t?
Good idea!
> > +
> > +static int hypfs_open(struct inode *inode, struct file *filp)
> > +{
> > + char *data = (char *)filp->f_dentry->d_inode->u.generic_ip;
>
> Unneeded typecast.
Yes
> > + if (filp->f_mode & FMODE_WRITE) {
> > + if (!(inode->i_mode & S_IWUGO))
> > + return -EACCES;
> > + }
> > + if (filp->f_mode & FMODE_READ) {
> > + if (!(inode->i_mode & S_IRUGO))
> > + return -EACCES;
> > + }
>
> Is the standard VFS permission checking not appropriate?
>
> (A comment should be added here).
You mean using .permission in the inode operations
and using the generic_permission() function?
Currently I do not have own inode operations (and
I don't want to have them ...)
> > + mutex_lock(&hypfs_lock);
> > + filp->private_data = kmalloc(strlen(data) + 1, GFP_KERNEL);
> > + if (!filp->private_data) {
> > + mutex_unlock(&hypfs_lock);
> > + return -ENOMEM;
> > + }
> > + strcpy(filp->private_data, data);
>
> kstrdup()
Yes!
> > + mutex_unlock(&hypfs_lock);
> > + return 0;
> > +}
> > +
> > +static ssize_t hypfs_aio_read(struct kiocb *iocb, __user char *buf,
> > + size_t count, loff_t offset)
> > +{
> > + char *data;
> > + size_t len;
> > + struct file *filp = iocb->ki_filp;
> > +
> > + data = (char *)filp->private_data;
>
> Unneeded cast.
Yes!
> > + len = strlen(data);
> > + if (offset > len) {
> > + count = 0;
> > + goto out;
> > + }
> > + if (count > len - offset)
> > + count = len - offset;
> > + if (copy_to_user(buf, data + offset, count)) {
> > + count = -EFAULT;
> > + goto out;
> > + }
> > + iocb->ki_pos += count;
> > + file_accessed(filp);
> > + out:
> > + return count;
> > +}
> > +static ssize_t hypfs_aio_write(struct kiocb *iocb, const char __user
*buf,
> > + size_t count, loff_t pos)
> > +{
> > + int rc;
> > + struct super_block *sb;
> > +
> > + mutex_lock(&hypfs_lock);
> > + sb = iocb->ki_filp->f_dentry->d_inode->i_sb;
> > + if (last_update_time == get_seconds()) {
> > + rc = -EBUSY;
> > + goto out;
> > + }
>
> Why's it looking at the time in this manner?
>
> (Whatever the reason, a comment should be added here).
I added the following comment:
/*
* Currently we only allow one update per second for two reasons:
* 1. diag 204 is VERY expensive
* 2. If several processes do updates in parallel and then read the
* hypfs data, the likelihood of collisions is reduced, if we restrict
* the minimum update interval. A collision occurs, if during the
* data gathering of one process another process triggers an update
* If the first process wants to ensure consistent data, it has
* to restart data collection in this case.
*/
[snip]
> > +static struct dentry *hypfs_create_update_file(struct super_block *sb,
> > + struct dentry *dir)
> > +{
> > + struct dentry *dentry;
> > +
> > + dentry = hypfs_create_file(sb, dir, "update", NULL,
> > + S_IFREG | UPDATE_FILE_MODE);
> > + if (!dentry)
> > + return ERR_PTR(-ENOMEM);
>
> But hypfs_create_file() will return ERR_PTR(-ENOMEM), never NULL.
Fixed this!
> > + /*
> > + * We do not put the update file on the 'delete' list with
> > + * hypfs_add_dentry(), since it should not be removed when the tree
> > + * is updated.
> > + */
> > + return dentry;
> > +}
> > +
> > +struct dentry *hypfs_create_u64(struct super_block *sb, struct dentry
*dir,
> > + const char *name, __u64 value)
> > +{
> > + char *buffer;
> > + char tmp[TMP_SIZE];
> > + struct dentry *dentry;
> > +
> > + snprintf(tmp, TMP_SIZE, "%lld\n", (unsigned long long int)value);
> > + buffer = kmalloc(strlen(tmp) + 1, GFP_KERNEL);
> > + if (!buffer)
> > + return ERR_PTR(-ENOMEM);
> > + strcpy(buffer, tmp);
>
> kstrdup()
Yes!
> > + dentry =
> > + hypfs_create_file(sb, dir, name, buffer, S_IFREG |
REG_FILE_MODE);
> > + if (!dentry) {
>
> IS_ERR()
Yes!
> > + kfree(buffer);
> > + return ERR_PTR(-ENOMEM);
> > + }
> > + hypfs_add_dentry(dentry);
> > + return dentry;
> > +}
> > +
> > +struct dentry *hypfs_create_str(struct super_block *sb, struct dentry
*dir,
> > + const char *name, char *string)
> > +{
> > + char *buffer;
> > + struct dentry *dentry;
> > +
> > + buffer = kmalloc(strlen(string) + 2, GFP_KERNEL);
> > + if (!buffer)
> > + return ERR_PTR(-ENOMEM);
> > + sprintf(buffer, "%s\n", string);
> > + dentry =
> > + hypfs_create_file(sb, dir, name, buffer, S_IFREG |
REG_FILE_MODE);
> > + if (!dentry) {
>
> IS_ERR()
> (Better check the whole patch for this)
Yes!
Thanks again for the review!
Michael
-
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]
