From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christoph Hellwig Subject: Re: [Qemu-devel] [PATCH 2/2] Add serial number support for virtio_blk, V3 Date: Wed, 27 May 2009 09:49:19 +0200 Message-ID: <20090527074919.GB7356@lst.de> References: <4A1C88DE.6050608@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: KVM list , qemu-devel@nongnu.org, rusty@rustcorp.com.au To: john cooper Return-path: Received: from verein.lst.de ([213.95.11.210]:55697 "EHLO verein.lst.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1760661AbZE0Hta (ORCPT ); Wed, 27 May 2009 03:49:30 -0400 Content-Disposition: inline In-Reply-To: <4A1C88DE.6050608@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: You should probably include rusty as he's collecting the patches for the virtio guest drivers. Also can you send the patch inline next time? That makes quoting it for review a lot easier. drivers/block/virtio_blk.c | 32 +++++++++++++++++++++++++++++--- include/linux/virtio_blk.h | 6 ++++++ 2 files changed, 35 insertions(+), 3 deletions(-) ================================================================= --- a/drivers/block/virtio_blk.c +++ b/drivers/block/virtio_blk.c @@ -146,12 +146,37 @@ static void do_virtblk_request(struct re vblk->vq->vq_ops->kick(vblk->vq); } +/* return serial# in IDE identify data (all other fields are currently zero) + */ +static int virtblk_identity(struct block_device *bdev, void *buf) +{ + struct virtio_blk *vblk = bdev->bd_disk->private_data; + u16 *id; + int rv; + + if (!(id = kzalloc(ATA_ID_WORDS, GFP_KERNEL))) + rv = -ENOMEM; + else if ((rv = virtio_config_buf(vblk->vdev, VIRTIO_BLK_F_SN, + offsetof(struct virtio_blk_config, serial), &id[ATA_ID_SERNO], + ATA_ID_SERNO_LEN))) + ; + else if (copy_to_user(buf, id, ATA_ID_WORDS)) + rv = -EFAULT; + else + rv = 0; + if (id) + kfree(id); + return (rv); +} + static int virtblk_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd, unsigned long data) { - return scsi_cmd_ioctl(bdev->bd_disk->queue, - bdev->bd_disk, mode, cmd, - (void __user *)data); + if (cmd == HDIO_GET_IDENTITY) + return (virtblk_identity(bdev, (void __user *)data)); + else + return scsi_cmd_ioctl(bdev->bd_disk->queue, bdev->bd_disk, + mode, cmd, (void __user *)data); } This looks functionally correct, but pretty far from normal kernel coding style. I'd envision something like this instead: /* * IDE-compatible identify ioctl. * * Currenlyt only returns the serial number and leaves all other fields * zero. */ static int virtblk_identity(struct gendisk *disk, void *argp) { struct virtio_blk *vblk = disk->private_data; u16 *id; int error = -ENOMEM; id = kzalloc(ATA_ID_WORDS, GFP_KERNEL) if (!id) goto out; error = virtio_config_buf(vblk->vdev, VIRTIO_BLK_F_SN, offsetof(struct virtio_blk_config, serial), &id[ATA_ID_SERNO], ATA_ID_SERNO_LEN); if (error) goto out_kfree; if (copy_to_user(argp, id, ATA_ID_WORDS)) error = -EFAULT; out_kfree: kfree(id); out: return error; } static int virtblk_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd, unsigned long arg) { struct gendisk *disk = bdev->bd_disk; void __user *argp = (void __user *arg); switch (cmd) { case HDIO_GET_IDENTITY: return virtblk_identity(disk, argp); default: return scsi_cmd_ioctl(disk->queue, disk, mode, cmd, argp); } }