Re: [PATCH] nbd driver for 2.5+: enhanced diagnostics support

viro@parcelfarce.linux.theplanet.co.uk
Tue, 24 Jun 2003 03:59:59 +0100


On Mon, Jun 23, 2003 at 08:04:49PM -0600, Lou Langholtz wrote:
> This third patch (for enhancing diagnostics support) applies
> incrementally after my last LKML'd patch (for cosmetic changes). These
> changes introduce configurable KERN_DEBUG level printk output for a
> variety of different things that the driver does and provides the
> framework for enhanced future debugging support as well.
>
> As always, comments welcome!

> + case BLKROSET: return "set-read-only";
> + case BLKROGET: return "get-read-only";
> + case BLKGETSIZE: return "block-get-size";
> + case BLKFLSBUF: return "flush-buffer-cache";

The last 4 never make it to the driver (nor should they, being generic
ioctls).

> + reply.magic = ntohl(reply.magic);
> + if (reply.magic != NBD_REPLY_MAGIC) {
> + printk(KERN_ERR "%s: Wrong magic (0x%lx)\n",
> + lo->disk->disk_name,
> + (unsigned long)reply.magic);

> + reply.error = ntohl(reply.error);
> + if (reply.error) {
> + printk(KERN_ERR "%s: Other side returned error (%d)\n",
> + lo->disk->disk_name, reply.error);

Generally a bad taste - it's harmless in this case, but such endianness
conversions in place are asking for trouble.

> lo->refcnt--;
> + dprintk(DBG_RELEASE, "%s: %s refcnt=%d\n", lo->disk->disk_name,
> + __FUNCTION__, lo->refcnt);
> /* N.B. Doesn't lo->file need an fput?? */
> return 0;

a) please, lose __FUNCTION__ - using it in macros is one thing, but
directly in a function that got a name shorter than `__FUNCTION__'?

b) no, it doesn't.

c) ->refcnt is never used. How about losing it completely? Along with
->open() and ->release()...

> sreq.flags = REQ_SPECIAL;
> nbd_cmd(&sreq) = NBD_CMD_DISC;

;-/

> + sreq.sector = 0;
> + sreq.nr_sectors = 0;

Umm... What for?

> if (sizeof(struct nbd_request) != 28) {
> - printk(KERN_CRIT "Sizeof nbd_request needs to be 28 in order to work!\n" );
> + printk(KERN_CRIT "nbd: Sizeof nbd_request needs to be 28 in order to work!\n" );
> return -EIO;
> }

FWIW, I'm less than sure that struct nbd_request is worth defining.
We use it in two places - the check above and nbd_send_req() where
it is filled and then its address is cast to char *. It might be
better to use u32[7] directly and forget about all alignment issues.
Hell knows... In this situation I would probably just define an
enum for offsets and be done with that. Same goes for nbd_reply.
-
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/