Re: [Qemu-devel] [PATCH 2/2] Add serial number support for virtio_blk, V3

[Christoph Hellwig <hch@lst.de>]

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);
	}
}