lkml.org 
[lkml]   [2003]   [Jun]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC][PATCH] nbd driver for 2.5.72
    On Fri, Jun 20, 2003 at 11:43:39PM -0600, Lou Langholtz wrote:

    In addition to comments from Andrew:

    > +#define MAJOR_NR NBD_MAJOR

    What for? We used to have very sick macros that depended on MAJOR_NR;
    they are gone. Lose it.

    > +#define NBD_DEBUG_OPEN 0x0001
    > +#define NBD_DEBUG_RELEASE 0x0002
    > +#define NBD_DEBUG_IOCTL 0x0004
    > +#define NBD_DEBUG_MEDIA 0x0008
    > +#define NBD_DEBUG_THREADS 0x0010
    > +#define NBD_DEBUG_SESSION 0x0020
    > +#define NBD_DEBUG_INIT 0x0040
    > +#define NBD_DEBUG_EXIT 0x0080
    > +#define NBD_DEBUG_RX 0x0100
    > +#define NBD_DEBUG_TX 0x0200
    > +#define NBD_DEBUG_BLKDEV 0x0400

    enum

    > +#define DEVICE_TO_MINOR(lo) ((int)((lo)-nbd_devs))

    lo->disk->disk_name would cover all uses of that animal (you always use
    it in printk, AFAICS, and always in the same way).

    > +# define REQUEST_QUEUE(req) (&(req)->queuelist)
    > +# define REQUEST_QUEUE_NEXT_REQUEST(q) (elv_next_request(q))
    > +# define REQUEST_CMD(req) ((req)->cmd[0])
    > +# define DAEMONIZE(fmt...) daemonize(fmt)
    > +# define NBD_BYTESIZE(lo) ((lo)->bytesize)
    > +# define NBD_BLKSIZE(lo) ((lo)->blksize)
    > +# define INODE_TO_NBD(i) ((i)->i_bdev->bd_disk->private_data)
    > +# define DEVICE_NAME "nbd"
    > +# define request_queue_lock(q) spin_lock_irq((q)->queue_lock)
    > +# define request_queue_unlock(q) spin_unlock_irq((q)->queue_lock)
    > +# define request_queue_lock_save(q,flags) \
    > + spin_lock_irqsave((q)->queue_lock, (flags))
    > +# define request_queue_unlock_restore(q,flags) \
    > + spin_unlock_irqrestore((q)->queue_lock, (flags))

    Lose them, they only obfuscate the code. inode_to_nbd() might make sense
    as static inline; the rest is definitely noise.

    > +static nbd_device_t nbd_devs[MAX_NBD];
    > +static struct request_queue nbd_queue[MAX_NBD];
    > +static spinlock_t nbd_lock[MAX_NBD];

    Why not put these into nbd_device?

    > +static uint32_t request_magic;

    ??? htonl(NBD_REQUEST_MAGIC) is perfectly OK in the place where you
    use it and more likely than not will give better code.

    > +static uint32_t reply_magic;

    Ditto.

    > +static struct block_device_operations nbd_fops =
    > +{
    > + owner: THIS_MODULE,
    > + open: nbd_open,
    > + release: nbd_release,
    > + ioctl: nbd_ioctl,
    > +};

    C99 syntax, please.

    > +static int nbd_debug_write_proc(struct file *file, const char *buffer,
    > + unsigned long count, void *data)
    > +{
    > + unsigned int newflags;
    > +
    > + if (!capable(CAP_SYS_ADMIN))
    > + return -EACCES;
    > + if (sscanf(buffer, "%x", &newflags) != 1)
    > + return -EINVAL;

    *ahem*

    First of all, "buffer" is userland pointer. At the very lease,
    copy_from_user() (and sanity checks on 'count') are missing. What's
    more, you are using sscanf() on something that doesn't have to be
    NUL-terminated. Limit count, copy into buffer, put '\0' into the last
    byte of said buffer and *then* use sscanf().

    > +static void __init mk_debug_proc_entry(void)
    > +{
    > + proc_debug = create_proc_entry("debugflags", S_IFREG|S_IRUSR|S_IWUSR,
    > + proc_array);
    > + if (!proc_debug)
    > + return;
    > + proc_debug->nlink = 1;
    > + proc_debug->data = NULL;

    ???

    > + proc_debug->read_proc = nbd_debug_read_proc;
    > + proc_debug->write_proc = nbd_debug_write_proc;
    > +}

    ->owner is missing.

    > +static void __init init_nbd_dev(nbd_device_t *dev)
    > +{
    > +#ifdef PARANOIA
    > + dev->magic = LO_MAGIC;
    > +#endif
    > + atomic_set(&dev->refcnt, 0);
    > + dev->flags = 0;
    > + spin_lock_init(&dev->lock);
    > + dev->harderror = 0;
    > + nbd_qsys_init(&dev->tx_queue);
    > + nbd_qsys_init(&dev->rx_queue);
    > + dev->file = NULL;
    > + dev->sock = NULL;
    > + memset(&dev->sin, 0, sizeof(dev->sin));
    > + dev->errcnt = 0;
    > + dev->lasterr = ENOTCONN;
    > + dev->closed = (RCV_SHUTDOWN|SEND_SHUTDOWN);
    > + dev->ss_thread.task = NULL;
    > + dev->tx_thread.task = NULL;
    > + dev->rx_thread.task = NULL;
    > + atomic_set(&dev->num_io_threads, 0);

    *Ugh*. These suckers are already zero-filled.

    > + init_waitqueue_head(&dev->no_io_waiters);
    > + init_MUTEX(&dev->semalock);
    > + dev->blksize = 1 << initial_blksize_bits;
    > + dev->bytesize = initial_bytesize;
    > + dev->disk = nbd_alloc_disk(dev);
    > + if (dev->disk)
    > + add_disk(dev->disk);
    > +}

    > +static inline void parse_sin(char *str, struct sockaddr_in *sin)
    > +{
    > + char *s = str;
    > + u16 port = default_port;
    > +
    > + /* parse format like: "10.0.0.5[:30666]" */
    > + while (*s && *s != ':')
    > + s++;

    man strchr

    > + requests_out = 0;
    > + qhandler_loops = 0;

    Static variables that do not have explicit initializer are initialized with 0.

    > +/* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
    > + *
    > + * End of definitions shared with user space.
    > + * From here on out, these definitions are only for kernel (driver).
    > + */
    > +
    > +#ifdef MAJOR_NR

    a) that's ifdef __KERNEL__
    b) use separate headers.
    -
    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 13:36    [W:0.068 / U:28.824 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site