Re: [PATCH v7 42/46] virtio_scsi: v1.0 support

On Mon, Dec 01, 2014 at 02:53:05PM +0200, Michael S. Tsirkin wrote:

On Mon, Dec 01, 2014 at 01:50:01PM +0100, Cornelia Huck wrote:

quoted

On Sun, 30 Nov 2014 17:12:47 +0200
"Michael S. Tsirkin" [off-list ref] wrote:

quoted

Note: for consistency, and to avoid sparse errors,
  convert all fields, even those no longer in use
  for virtio v1.0.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/linux/virtio_scsi.h | 32 +++++++++++++++-------------
 drivers/scsi/virtio_scsi.c  | 51 ++++++++++++++++++++++++++++-----------------
 2 files changed, 49 insertions(+), 34 deletions(-)

quoted

@@ -196,10 +196,13 @@ static void virtscsi_complete_cmd(struct virtio_scsi *vscsi, void *buf)
 		break;
 	}

-	WARN_ON(resp->sense_len > VIRTIO_SCSI_SENSE_SIZE);
+	WARN_ON(virtio32_to_cpu(vscsi->vdev, resp->sense_len) >
+		VIRTIO_SCSI_SENSE_SIZE);

Introduce a local variable for this? Might make this statement and the
min_t statement below easier to read.

I prefer 1:1 code conversions. We can do refactorings on top.
Since you mention this line as hard to read, I'll just make it > 80
chars for now, and trivial line splits can come later.
OK?

Or maybe I'll keep it as is, since Paolo who wrote this code
is happy with it ..

quoted

quoted

 	if (sc->sense_buffer) {
 		memcpy(sc->sense_buffer, resp->sense,
-		       min_t(u32, resp->sense_len, VIRTIO_SCSI_SENSE_SIZE));
+		       min_t(u32,
+			     virtio32_to_cpu(vscsi->vdev, resp->sense_len),
+			     VIRTIO_SCSI_SENSE_SIZE));
 		if (resp->sense_len)
 			set_driver_byte(sc, DRIVER_SENSE);
 	}

Otherwise looks good to me.