[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PULL 40/96] virtio-scsi: use standard-headers
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PULL 40/96] virtio-scsi: use standard-headers |
Date: |
Wed, 11 Mar 2015 13:25:50 +0100 |
On Wed, Mar 11, 2015 at 10:48:27AM +0100, Paolo Bonzini wrote:
>
>
> On 11/03/2015 09:31, Michael S. Tsirkin wrote:
> > 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 <address@hidden>
> >>> Acked-by: Paolo Bonzini <address@hidden>
> >>
> >> 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));
>
> No, this is too ugly. Can I just queue a revert?
>
> Paolo
How about we just drop this chunk? It's not checking anything useful
now.
Others are good?
> > #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 ||
> >
> >