From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53527) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WnNtH-0004NN-TC for qemu-devel@nongnu.org; Thu, 22 May 2014 03:58:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WnNtB-0002pb-Mx for qemu-devel@nongnu.org; Thu, 22 May 2014 03:58:15 -0400 Received: from mx1.redhat.com ([209.132.183.28]:26148) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WnNtB-0002p1-DB for qemu-devel@nongnu.org; Thu, 22 May 2014 03:58:09 -0400 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s4M7w7Dv015705 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 22 May 2014 03:58:07 -0400 Message-ID: <537DAE0C.7030308@redhat.com> Date: Thu, 22 May 2014 09:58:04 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1400744232-29173-1-git-send-email-famz@redhat.com> <1400744232-29173-2-git-send-email-famz@redhat.com> In-Reply-To: <1400744232-29173-2-git-send-email-famz@redhat.com> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 1/2] virtio-blk: Factor out virtio_blk_handle_scsi_req from virtio_blk_handle_scsi List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng , qemu-devel@nongnu.org Cc: Stefan Hajnoczi Il 22/05/2014 09:37, Fam Zheng ha scritto: > -static void virtio_blk_handle_scsi(VirtIOBlockReq *req) > +int virtio_blk_handle_scsi_req(BlockDriverState *bs, > + VirtIOBlkConf *conf, > + VirtQueueElement *elem) Two more comments... please pass the VirtIOBlock * instead of bs/conf here. > { > + int status = VIRTIO_BLK_S_OK; > + struct virtio_scsi_inhdr *scsi = NULL; > #ifdef __linux__ > - int ret; > int i; > + struct sg_io_hdr hdr; > #endif > - int status = VIRTIO_BLK_S_OK; > > /* > * We require at least one output segment each for the virtio_blk_outhdr > @@ -140,63 +142,61 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req) > * We also at least require the virtio_blk_inhdr, the virtio_scsi_inhdr > * and the sense buffer pointer in the input segments. > */ > - if (req->elem.out_num < 2 || req->elem.in_num < 3) { > - virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR); > - g_free(req); > - return; > + if (elem->out_num < 2 || elem->in_num < 3) { > + status = VIRTIO_BLK_S_IOERR; > + goto fail; > } > > /* > * The scsi inhdr is placed in the second-to-last input segment, just > * before the regular inhdr. > */ > - req->scsi = (void *)req->elem.in_sg[req->elem.in_num - 2].iov_base; > + scsi = (void *)elem->in_sg[elem->in_num - 2].iov_base; > > - if (!req->dev->blk.scsi) { > + /* > + * No support for bidirection commands yet. > + */ > + if (elem->out_num > 2 && elem->in_num > 3) { > status = VIRTIO_BLK_S_UNSUPP; > goto fail; > } > > - /* > - * No support for bidirection commands yet. > - */ > - if (req->elem.out_num > 2 && req->elem.in_num > 3) { > + if (!conf->scsi) { > status = VIRTIO_BLK_S_UNSUPP; > goto fail; > } Swapping the two conditionals unnecessarily makes the patch bigger. Paolo > #ifdef __linux__ > - struct sg_io_hdr hdr; > memset(&hdr, 0, sizeof(struct sg_io_hdr)); > hdr.interface_id = 'S'; > - hdr.cmd_len = req->elem.out_sg[1].iov_len; > - hdr.cmdp = req->elem.out_sg[1].iov_base; > + hdr.cmd_len = elem->out_sg[1].iov_len; > + hdr.cmdp = elem->out_sg[1].iov_base; > hdr.dxfer_len = 0; > > - if (req->elem.out_num > 2) { > + if (elem->out_num > 2) { > /* > * If there are more than the minimally required 2 output segments > * there is write payload starting from the third iovec. > */ > hdr.dxfer_direction = SG_DXFER_TO_DEV; > - hdr.iovec_count = req->elem.out_num - 2; > + hdr.iovec_count = elem->out_num - 2; > > for (i = 0; i < hdr.iovec_count; i++) > - hdr.dxfer_len += req->elem.out_sg[i + 2].iov_len; > + hdr.dxfer_len += elem->out_sg[i + 2].iov_len; > > - hdr.dxferp = req->elem.out_sg + 2; > + hdr.dxferp = elem->out_sg + 2; > > - } else if (req->elem.in_num > 3) { > + } else if (elem->in_num > 3) { > /* > * If we have more than 3 input segments the guest wants to actually > * read data. > */ > hdr.dxfer_direction = SG_DXFER_FROM_DEV; > - hdr.iovec_count = req->elem.in_num - 3; > + hdr.iovec_count = elem->in_num - 3; > for (i = 0; i < hdr.iovec_count; i++) > - hdr.dxfer_len += req->elem.in_sg[i].iov_len; > + hdr.dxfer_len += elem->in_sg[i].iov_len; > > - hdr.dxferp = req->elem.in_sg; > + hdr.dxferp = elem->in_sg; > } else { > /* > * Some SCSI commands don't actually transfer any data. > @@ -204,11 +204,11 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req) > hdr.dxfer_direction = SG_DXFER_NONE; > } > > - hdr.sbp = req->elem.in_sg[req->elem.in_num - 3].iov_base; > - hdr.mx_sb_len = req->elem.in_sg[req->elem.in_num - 3].iov_len; > + hdr.sbp = elem->in_sg[elem->in_num - 3].iov_base; > + hdr.mx_sb_len = elem->in_sg[elem->in_num - 3].iov_len; > > - ret = bdrv_ioctl(req->dev->bs, SG_IO, &hdr); > - if (ret) { > + status = bdrv_ioctl(bs, SG_IO, &hdr); > + if (status) { > status = VIRTIO_BLK_S_UNSUPP; > goto fail; > } > @@ -224,23 +224,32 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req) > hdr.status = CHECK_CONDITION; > } > > - stl_p(&req->scsi->errors, > + stl_p(&scsi->errors, > hdr.status | (hdr.msg_status << 8) | > (hdr.host_status << 16) | (hdr.driver_status << 24)); > - stl_p(&req->scsi->residual, hdr.resid); > - stl_p(&req->scsi->sense_len, hdr.sb_len_wr); > - stl_p(&req->scsi->data_len, hdr.dxfer_len); > + stl_p(&scsi->residual, hdr.resid); > + stl_p(&scsi->sense_len, hdr.sb_len_wr); > + stl_p(&scsi->data_len, hdr.dxfer_len); > > - virtio_blk_req_complete(req, status); > - g_free(req); > - return; > + return status; > #else > abort(); > #endif > > fail: > /* Just put anything nonzero so that the ioctl fails in the guest. */ > - stl_p(&req->scsi->errors, 255); > + if (scsi) { > + stl_p(&scsi->errors, 255); > + } > + return status; > +} > + > +static void virtio_blk_handle_scsi(VirtIOBlockReq *req) > +{ > + int status; > + > + status = virtio_blk_handle_scsi_req(req->dev->bs, &req->dev->blk, > + &req->elem); > virtio_blk_req_complete(req, status); > g_free(req); > } > diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h > index e4c41ff..1b00b48 100644 > --- a/include/hw/virtio/virtio-blk.h > +++ b/include/hw/virtio/virtio-blk.h > @@ -155,4 +155,8 @@ typedef struct VirtIOBlock { > > void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk); > > +int virtio_blk_handle_scsi_req(BlockDriverState *bs, > + VirtIOBlkConf *conf, > + VirtQueueElement *elem); > + > #endif >