lkml.org 
[lkml]   [2004]   [Dec]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] ATA over Ethernet driver for 2.6.9
    Hi Ed,

    Few comments below.

    On Mon, 06 Dec 2004 10:51:46 -0500, Ed L Cashin <ecashin@coraid.com> wrote:
    > diff -urpN linux-2.6.9/drivers/block/aoe/all.h linux-2.6.9-aoe/drivers/block/aoe/all.h
    > --- linux-2.6.9/drivers/block/aoe/all.h 1969-12-31 19:00:00.000000000 -0500
    > +++ linux-2.6.9-aoe/drivers/block/aoe/all.h 2004-12-06 10:40:00.000000000 -0500

    How about calling this aoe.h or similar?

    > @@ -0,0 +1,156 @@
    > +#include <linux/ctype.h>
    > +#include <linux/string.h>
    > +#include <linux/hdreg.h>
    > +#include <linux/blkdev.h>
    > +#include <linux/types.h>
    > +#include <linux/fs.h>
    > +#include <linux/module.h>
    > +#include <linux/blkpg.h>
    > +#include <linux/slab.h>
    > +#include <linux/skbuff.h>
    > +#include <linux/ioctl.h>
    > +#include <linux/if.h>
    > +#include <linux/netdevice.h>
    > +#include <linux/kdev_t.h>
    > +#include <linux/kernel.h>
    > +#include <linux/init.h>
    > +#include <linux/poll.h>
    > +#include <linux/timer.h>
    > +#include <linux/genhd.h>
    > +#include <asm/uaccess.h>
    > +#include <asm/namei.h>
    > +#include <asm/semaphore.h>

    This looks wrong. I don't see you using, for example, semaphores
    everywhere you include this header. Please just move these includes to
    the appropriate .c files.

    > +enum {
    > + DEVFL_UP = 1, /* device is installed in system and ready for AoE->ATA commands */
    > + DEVFL_TKILL = (1<<1), /* flag for timer to know when to kill self */
    > + DEVFL_EXT = (1<<2), /* device accepts lba48 commands */
    > + DEVFL_CLOSEWAIT = (1<<3), /* device is waiting for all closes to revalidate */
    > + DEVFL_WC_UPDATE = (1<<4), /* this device needs to update write cache status */
    > + DEVFL_WORKON = (1<<4),
    > +
    > + BUFFL_FAIL = 1,
    > +
    > + MAXATADATA = 1024,
    > + NPERSHELF = 10,
    > + FREETAG = -1,
    > +};

    Perhaps it would be cleaner if you kept DEVFL flags in separate enum
    block as they're related and have nothing to do with the rest?

    > +typedef struct Buf Buf;
    > +struct Buf {

    Please don't introduce typedefs for structs. Furthermore, please make
    the struct name start with a lower case letter like the rest of the
    kernel.

    > diff -urpN linux-2.6.9/drivers/block/aoe/aoeblk.c linux-2.6.9-aoe/drivers/block/aoe/aoeblk.c
    > --- linux-2.6.9/drivers/block/aoe/aoeblk.c 1969-12-31 19:00:00.000000000 -0500
    > +++ linux-2.6.9-aoe/drivers/block/aoe/aoeblk.c 2004-12-06 10:40:00.000000000 -0500
    > +static int
    > +aoeblk_open(struct inode *inode, struct file *filp)
    > +{
    > + Aoedev *d;
    > + ulong flags;
    > +
    > + d = (Aoedev *) inode->i_bdev->bd_disk->private_data;

    Please remove the redundant casts when converting from a void pointer.
    The conversion is guaranteed by the C standard.

    Pekka
    -
    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.028 / U:91.000 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site