diff mbox

[v2] iscsi: use scsi_create_task()

Message ID 20170728163004.21905-1-marcandre.lureau@redhat.com
State New
Headers show

Commit Message

Marc-André Lureau July 28, 2017, 4:30 p.m. UTC 
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 | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

v2:
 - set cdb_size if API_VERSION < 20150510

Comments

Paolo Bonzini July 28, 2017, 4:58 p.m. UTC | #1 
On 28/07/2017 18:30, 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.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  block/iscsi.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)

Stupid question: what's the benefit?  Change malloc to g_new0 in the
existing code, and we even make it shorter...

Paolo

> v2:
>  - set cdb_size if API_VERSION < 20150510
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index d557c99668..9449c90631 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,31 +1035,30 @@ 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;
> +    }
>  
> +#if LIBISCSI_API_VERSION < 20150510
> +    acb->task->cdb_size = acb->ioh->cmd_len; /* fixed in libiscsi 1.13.0 */
> +#endif
>      data.size = 0;
>      qemu_mutex_lock(&iscsilun->mutex);
>      if (acb->task->xfer_dir == SCSI_XFER_WRITE) {
>
Marc-André Lureau July 28, 2017, 5:52 p.m. UTC | #2 
On Fri, Jul 28, 2017 at 6:58 PM Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 28/07/2017 18:30, 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.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  block/iscsi.c | 30 +++++++++++++++---------------
> >  1 file changed, 15 insertions(+), 15 deletions(-)
>
> Stupid question: what's the benefit?


scsi_create/scsi_free is a library API. If they have their own allocator,
we better use it, or it may easily break, no?


> Change malloc to g_new0 in the
> existing code, and we even make it shorter...
>

replacing malloc with g_new is the subject of another upcoming series :) (
https://github.com/elmarco/clang-tools-extra/blob/master/clang-tidy/qemu/UseGnewCheck.cpp
)


>
> Paolo
>
> > v2:
> >  - set cdb_size if API_VERSION < 20150510
> >
> > diff --git a/block/iscsi.c b/block/iscsi.c
> > index d557c99668..9449c90631 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,31 +1035,30 @@ 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;
> > +    }
> >
> > +#if LIBISCSI_API_VERSION < 20150510
> > +    acb->task->cdb_size = acb->ioh->cmd_len; /* fixed in libiscsi
> 1.13.0 */
> > +#endif
> >      data.size = 0;
> >      qemu_mutex_lock(&iscsilun->mutex);
> >      if (acb->task->xfer_dir == SCSI_XFER_WRITE) {
> >
>
>
> --
Marc-André Lureau
Paolo Bonzini Aug. 1, 2017, 1:03 p.m. UTC | #3 
On 28/07/2017 19:52, Marc-André Lureau wrote:
> 
>     Stupid question: what's the benefit?  
> 
> scsi_create/scsi_free is a library API. If they have their own
> allocator, we better use it, or it may easily break, no?

Well, that would be an API breakage, but I see the point.  I think I
would prefer something like

static inline struct scsi_task *iscsi_create_task(...)
{
#if ...
    return scsi_create_task(...)
#else
    /* Older versions of libiscsi have a bug, so include our
     * implementation.
     */
    struct scsi_task *task = g_try_malloc0(sizeof(struct scsi_task));
    if (!task) {
        task;
    }

    memcpy(&task->cdb[0], cdb, cdb_size);
    task->cdb_size   = cdb_size;
    task->xfer_dir   = xfer_dir;
    task->expxferlen = expxferlen;
    return task;
#endif
}

> 
>     Change malloc to g_new0 in the
>     existing code, and we even make it shorter...
> 
> 
> replacing malloc with g_new is the subject of another upcoming series :)
> (https://github.com/elmarco/clang-tools-extra/blob/master/clang-tidy/qemu/UseGnewCheck.cpp)
>
diff mbox

Patch

diff --git a/block/iscsi.c b/block/iscsi.c
index d557c99668..9449c90631 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,31 +1035,30 @@  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;
+    }
 
+#if LIBISCSI_API_VERSION < 20150510
+    acb->task->cdb_size = acb->ioh->cmd_len; /* fixed in libiscsi 1.13.0 */
+#endif
     data.size = 0;
     qemu_mutex_lock(&iscsilun->mutex);
     if (acb->task->xfer_dir == SCSI_XFER_WRITE) {