[Qemu-devel] [PATCH] iscsi: use scsi_create_task()

[Qemu-devel] [PATCH] iscsi: use scsi_create_task()

[Marc-André Lureau]

The function does the same initialization, and matches with
scsi_free_scsi_task() usage, and qemu doesn't need to know the
allocator details.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 block/iscsi.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 5daa201181..a6bcd8eb13 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1013,6 +1013,7 @@ static BlockAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
     struct iscsi_context *iscsi = iscsilun->iscsi;
     struct iscsi_data data;
     IscsiAIOCB *acb;
+    int xfer_dir;
 
     acb = qemu_aio_get(&iscsi_aiocb_info, bs, cb, opaque);
 
@@ -1034,30 +1035,26 @@ static BlockAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
         return NULL;
     }
 
-    acb->task = malloc(sizeof(struct scsi_task));
-    if (acb->task == NULL) {
-        error_report("iSCSI: Failed to allocate task for scsi command. %s",
-                     iscsi_get_error(iscsi));
-        qemu_aio_unref(acb);
-        return NULL;
-    }
-    memset(acb->task, 0, sizeof(struct scsi_task));
-
     switch (acb->ioh->dxfer_direction) {
     case SG_DXFER_TO_DEV:
-        acb->task->xfer_dir = SCSI_XFER_WRITE;
+        xfer_dir = SCSI_XFER_WRITE;
         break;
     case SG_DXFER_FROM_DEV:
-        acb->task->xfer_dir = SCSI_XFER_READ;
+        xfer_dir = SCSI_XFER_READ;
         break;
     default:
-        acb->task->xfer_dir = SCSI_XFER_NONE;
+        xfer_dir = SCSI_XFER_NONE;
         break;
     }
 
-    acb->task->cdb_size = acb->ioh->cmd_len;
-    memcpy(&acb->task->cdb[0], acb->ioh->cmdp, acb->ioh->cmd_len);
-    acb->task->expxferlen = acb->ioh->dxfer_len;
+    acb->task = scsi_create_task(acb->ioh->cmd_len, acb->ioh->cmdp,
+                                 xfer_dir, acb->ioh->dxfer_len);
+    if (acb->task == NULL) {
+        error_report("iSCSI: Failed to allocate task for scsi command. %s",
+                     iscsi_get_error(iscsi));
+        qemu_aio_unref(acb);
+        return NULL;
+    }
 
     data.size = 0;
     qemu_mutex_lock(&iscsilun->mutex);
-- 
2.13.0.91.g00982b8dd

Re: [Qemu-devel] [Qemu-block] [PATCH] iscsi: use scsi_create_task()

[John Snow]

On 06/07/2017 02:35 AM, Marc-André Lureau wrote:
> The function does the same initialization, and matches with
> scsi_free_scsi_task() usage, and qemu doesn't need to know the
> allocator details.
> 

Happy month-iversary for this patch!

Yeah, uh, we should probably not be reaching into this structure
ourselves, huh? I had to look up the iscsi source just to confirm that
we /were/ doing the right thing, but now we won't have to worry about it.

> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: John Snow <jsnow@redhat.com>

This should be care of... Ronnie, Paolo, and Peter Lieven?

Re: [Qemu-devel] [Qemu-block] [PATCH] iscsi: use scsi_create_task()

[Paolo Bonzini]

On 06/07/2017 23:37, John Snow wrote:
> 
> On 06/07/2017 02:35 AM, Marc-André Lureau wrote:
>> The function does the same initialization, and matches with
>> scsi_free_scsi_task() usage, and qemu doesn't need to know the
>> allocator details.
>>
> 
> Happy month-iversary for this patch!
> 
> Yeah, uh, we should probably not be reaching into this structure
> ourselves, huh? I had to look up the iscsi source just to confirm that
> we /were/ doing the right thing, but now we won't have to worry about it.
> 
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Reviewed-by: John Snow <jsnow@redhat.com>
> 
> This should be care of... Ronnie, Paolo, and Peter Lieven?

The only snag:

commit 0c6e3bffb353f141bb68fe822e3ea9df5eee8c8c
Author: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Date:   Thu Dec 18 09:20:17 2014 -0800

    SCSI: scsi_create_task must set task->cdb_size
    
    We forgot to set task->cdb_size which means all tasks we create with this
    function becomes test unit ready :-(
    
    Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>


At least RHEL7 has libiscsi-1.9.0.

Paolo

Re: [Qemu-devel] [Qemu-block] [PATCH] iscsi: use scsi_create_task()

[Paolo Bonzini]

> Drats, backwards compatibility gets me again.
> 
> I guess it should become a comment in the code that explains why we
> cannot use the lib function for it?

Or take Marc-André's patch and copy the (fixed) function into QEMU.

> Tch, just trying to keep backlogged qemu-block patches from falling off
> the radar :)

I totally missed this one, so thank you.

Paolo