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/
This archive was generated by hypermail 2b29 : Mon Jun 23 2003 - 22:00:35 EST