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
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) {
--
2.14.0.rc0.1.g40ca67566
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) { >
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
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) >
© 2016 - 2024 Red Hat, Inc.