| Message ID | 20090527074919.GB7356@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 27, 2009 at 09:49:19AM +0200, Christoph Hellwig wrote: > /* > * IDE-compatible identify ioctl. > * > * Currenlyt only returns the serial number and leaves all other fields > * zero. > */ Btw, thinking about it the rest of the information in the ioctl should probably be filled up with faked data, similar to how we do it for the ide emulation inside qemu. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 27 May 2009 05:19:19 pm Christoph Hellwig wrote: > You should probably include rusty as he's collecting the patches > for the virtio guest drivers. Yes. It *does* help to cc the maintainer if you want your patches applied :) And I particularly love untested code like this! > + if (!(id = kzalloc(ATA_ID_WORDS, GFP_KERNEL))) > + rv = -ENOMEM; Doesn't ATA_ID_WORDS seem like a strange name for a number of bytes? What's this *for* BTW? Rusty. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Christoph Hellwig wrote: > This looks functionally correct, but pretty far from normal kernel coding > style. I tend to avoid 'goto's. Christoph Hellwig wrote: >> /* >> * IDE-compatible identify ioctl. >> * >> * Currenlyt only returns the serial number and leaves all other fields >> * zero. >> */ > > Btw, thinking about it the rest of the information in the ioctl should > probably be filled up with faked data, similar to how we do it for > the ide emulation inside qemu. Doing so crossed my mind but thought it may be better to start here and provide data on an as-needed basis. But as you point out there is precedent (and likely reason) for hw/ide.c:ide_identify() doing as such. I don't have a strong bias either way. Comments from others? -john
Christoph Hellwig wrote: > On Wed, May 27, 2009 at 09:49:19AM +0200, Christoph Hellwig wrote: >> /* >> * IDE-compatible identify ioctl. >> * >> * Currenlyt only returns the serial number and leaves all other fields >> * zero. >> */ > > Btw, thinking about it the rest of the information in the ioctl should > probably be filled up with faked data, similar to how we do it for > the ide emulation inside qemu. Agreed. I've done so to the extent it makes sense in the case of the more generic fields. A fair amount of the identify information being generated by hw/ide.c appears obsoleted. -john
>> + if (!(id = kzalloc(ATA_ID_WORDS, GFP_KERNEL))) >> + rv = -ENOMEM; > > Doesn't ATA_ID_WORDS seem like a strange name for a number of bytes? Yes I caught that bug in the rework as well. > What's this *for* BTW? Sorry -- I assumed you were on either list. Please see patch to follow. -john
================================================================= --- 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); }
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(-) 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); } } -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html