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

[Marc-André Lureau]

On Fri, Jul 28, 2017 at 6:58 PM Paolo Bonzini <address@hidden> 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 <address@hidden>
> > ---
> >  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