Re: [Qemu-devel] [PULL 40/96] virtio-scsi: use standard-headers

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Mar 11, 2015 at 03:14:16PM +0800, Fam Zheng wrote:
> On Wed, 02/18 22:48, Michael S. Tsirkin wrote:
> > Drop duplicated code.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Michael, this breaks virtio-scsi-pci:
> 
>     qemu-system-x86_64: wrong size for virtio-scsi headers

Testing this now.

diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index de2c739..25d96f3 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -136,15 +136,15 @@ typedef struct VirtIOSCSIReq {
     union {
         struct {
             VirtIOSCSICmdReq  cmd;
-            uint8_t           cdb[];
         } QEMU_PACKED;
         VirtIOSCSICtrlTMFReq  tmf;
         VirtIOSCSICtrlANReq   an;
     } req;
 } VirtIOSCSIReq;
 
-QEMU_BUILD_BUG_ON(offsetof(VirtIOSCSIReq, req.cdb) !=
-                  offsetof(VirtIOSCSIReq, req.cmd) + sizeof(VirtIOSCSICmdReq));
+QEMU_BUILD_BUG_ON(offsetof(VirtIOSCSIReq, req.cmd.cdb) !=
+                  offsetof(VirtIOSCSIReq, req.cmd) +
+                  offsetof(VirtIOSCSICmdReq, cdb));
 
 #define DEFINE_VIRTIO_SCSI_PROPERTIES(_state, _conf_field)                     \
     DEFINE_PROP_UINT32("num_queues", _state, _conf_field.num_queues, 1),       \
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index cfb52e8..52bc00c 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -47,7 +47,8 @@ VirtIOSCSIReq *virtio_scsi_init_req(VirtIOSCSI *s, VirtQueue *vq)
     const size_t zero_skip = offsetof(VirtIOSCSIReq, elem)
                              + sizeof(VirtQueueElement);
 
-    req = g_slice_alloc(sizeof(*req) + vs->cdb_size);
+    req = g_slice_alloc(sizeof(*req) + MAX(vs->cdb_size, VIRTIO_SCSI_CDB_SIZE) -
+                        VIRTIO_SCSI_CDB_SIZE);
     req->vq = vq;
     req->dev = s;
     qemu_sglist_init(&req->qsgl, DEVICE(s), 8, &address_space_memory);
@@ -62,7 +63,8 @@ void virtio_scsi_free_req(VirtIOSCSIReq *req)
 
     qemu_iovec_destroy(&req->resp_iov);
     qemu_sglist_destroy(&req->qsgl);
-    g_slice_free1(sizeof(*req) + vs->cdb_size, req);
+    g_slice_free1(sizeof(*req) + MAX(vs->cdb_size, VIRTIO_SCSI_CDB_SIZE) -
+                        VIRTIO_SCSI_CDB_SIZE, req);
 }
 
 static void virtio_scsi_complete_req(VirtIOSCSIReq *req)
@@ -213,8 +215,10 @@ static void *virtio_scsi_load_request(QEMUFile *f, SCSIRequest *sreq)
     assert(req->elem.in_num <= ARRAY_SIZE(req->elem.in_sg));
     assert(req->elem.out_num <= ARRAY_SIZE(req->elem.out_sg));
 
-    if (virtio_scsi_parse_req(req, sizeof(VirtIOSCSICmdReq) + vs->cdb_size,
-                              sizeof(VirtIOSCSICmdResp) + vs->sense_size) < 0) {
+    if (virtio_scsi_parse_req(req, offsetof(VirtIOSCSICmdReq, cdb) +
+                              vs->cdb_size,
+                              offsetof(VirtIOSCSICmdResp, sense) +
+                              vs->sense_size) < 0) {
         error_report("invalid SCSI request migration data");
         exit(1);
     }
@@ -439,7 +443,7 @@ static void virtio_scsi_complete_cmd_req(VirtIOSCSIReq *req)
     /* Sense data is not in req->resp and is copied separately
      * in virtio_scsi_command_complete.
      */
-    req->resp_size = sizeof(VirtIOSCSICmdResp);
+    req->resp_size = offsetof(VirtIOSCSICmdResp, sense);
     virtio_scsi_complete_req(req);
 }
 
@@ -462,8 +466,10 @@ static void virtio_scsi_command_complete(SCSIRequest *r, uint32_t status,
     } else {
         req->resp.cmd.resid = 0;
         sense_len = scsi_req_get_sense(r, sense, sizeof(sense));
-        sense_len = MIN(sense_len, req->resp_iov.size - sizeof(req->resp.cmd));
-        qemu_iovec_from_buf(&req->resp_iov, sizeof(req->resp.cmd),
+        sense_len = MIN(sense_len, req->resp_iov.size -
+                        offsetof(typeof(req->resp.cmd), sense));
+        qemu_iovec_from_buf(&req->resp_iov,
+                            offsetof(typeof(req->resp.cmd), sense),
                             sense, sense_len);
         req->resp.cmd.sense_len = virtio_tswap32(vdev, sense_len);
     }
@@ -522,8 +528,10 @@ bool virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
     SCSIDevice *d;
     int rc;
 
-    rc = virtio_scsi_parse_req(req, sizeof(VirtIOSCSICmdReq) + vs->cdb_size,
-                               sizeof(VirtIOSCSICmdResp) + vs->sense_size);
+    rc = virtio_scsi_parse_req(req, offsetof(VirtIOSCSICmdReq, cdb) +
+                               vs->cdb_size,
+                               offsetof(VirtIOSCSICmdResp, sense) +
+                               vs->sense_size);
     if (rc < 0) {
         if (rc == -ENOTSUP) {
             virtio_scsi_fail_cmd_req(req);
@@ -544,7 +552,7 @@ bool virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
     }
     req->sreq = scsi_req_new(d, req->req.cmd.tag,
                              virtio_scsi_get_lun(req->req.cmd.lun),
-                             req->req.cdb, req);
+                             req->req.cmd.cdb, req);
 
     if (req->sreq->cmd.mode != SCSI_XFER_NONE
         && (req->sreq->cmd.mode != req->mode ||