[v2] block/iscsi: Adding iser support in Libiscsi-QEMU

iSER is a new transport layer supported in Libiscsi,
iSER provides a zero-copy RDMA capable interface that can
improve performance.

New API is introduced in abstracion of the Libiscsi transport layer.
In order to use the new iSER transport, one need to add the ?iser option
at the end of Libiscsi URI.

For now iSER memory buffers are pre-allocated and pre-registered,
hence in order to work with iSER from QEMU, one need to enable MEMLOCK
attribute in the VM to be large enough for all iSER buffers and RDMA
resources.

A new functionallity is also introduced in this commit, a new API
to deploy zero-copy command submission. iSER is differing from TCP in
data-path, hence IO vectors must be transferred already when queueing
the PDU.

changes from v1:
- Adding iser as an additional block driver

Signed-off-by: Roy Shterman <roysh@mellanox.com>
---
 block/iscsi.c |   80 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 76 insertions(+), 4 deletions(-)

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 1474481984-10452-1-git-send-email-roysh@mellanox.com
Subject: [Qemu-devel] [PATCH v2] block/iscsi: Adding iser support in Libiscsi-QEMU

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/1474481984-10452-1-git-send-email-roysh@mellanox.com -> patchew/1474481984-10452-1-git-send-email-roysh@mellanox.com
Switched to a new branch 'test'
6a9d88e block/iscsi: Adding iser support in Libiscsi-QEMU

=== OUTPUT BEGIN ===
Checking PATCH 1/1: block/iscsi: Adding iser support in Libiscsi-QEMU...
WARNING: line over 80 characters
#48: FILE: block/iscsi.c:601:
+                                            NULL, num_sectors * iscsilun->block_size,

WARNING: line over 80 characters
#49: FILE: block/iscsi.c:602:
+                                            iscsilun->block_size, 0, 0, fua, 0, 0,

ERROR: line over 90 characters
#50: FILE: block/iscsi.c:603:
+                                            iscsi_co_generic_cb, &iTask, (struct scsi_iovec *)iov->iov, iov->niov);

WARNING: line over 80 characters
#53: FILE: block/iscsi.c:606:
+                                            NULL, num_sectors * iscsilun->block_size,

WARNING: line over 80 characters
#54: FILE: block/iscsi.c:607:
+                                            iscsilun->block_size, 0, 0, fua, 0, 0,

ERROR: line over 90 characters
#55: FILE: block/iscsi.c:608:
+                                            iscsi_co_generic_cb, &iTask, (struct scsi_iovec *)iov->iov, iov->niov);

ERROR: line over 90 characters
#84: FILE: block/iscsi.c:815:
+                                           iscsi_co_generic_cb, &iTask, (struct scsi_iovec *)iov->iov, iov->niov);

ERROR: line over 90 characters
#90: FILE: block/iscsi.c:821:
+                                           iscsi_co_generic_cb, &iTask, (struct scsi_iovec *)iov->iov, iov->niov);

WARNING: line over 80 characters
#116: FILE: block/iscsi.c:1626:
+    iscsi_url = iscsi_parse_full_url(iscsi, uri_string_unescape(filename, -1, NULL));

ERROR: line over 90 characters
#119: FILE: block/iscsi.c:1628:
+        error_setg(errp, "Failed to parse URL : %s", uri_string_unescape(filename, -1, NULL));

ERROR: architecture specific defines should be avoided
#168: FILE: block/iscsi.c:2076:
+#ifdef __linux__

total: 6 errors, 5 warnings, 144 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

On Wed, Sep 21, 2016 at 09:19:44PM +0300, Roy Shterman wrote:
> iSER is a new transport layer supported in Libiscsi,
> iSER provides a zero-copy RDMA capable interface that can
> improve performance.
> 
> New API is introduced in abstracion of the Libiscsi transport layer.
> In order to use the new iSER transport, one need to add the ?iser option
> at the end of Libiscsi URI.
> 
> For now iSER memory buffers are pre-allocated and pre-registered,
> hence in order to work with iSER from QEMU, one need to enable MEMLOCK
> attribute in the VM to be large enough for all iSER buffers and RDMA
> resources.
> 
> A new functionallity is also introduced in this commit, a new API
> to deploy zero-copy command submission. iSER is differing from TCP in
> data-path, hence IO vectors must be transferred already when queueing
> the PDU.
> 
> changes from v1:
> - Adding iser as an additional block driver
> 
> Signed-off-by: Roy Shterman <roysh@mellanox.com>
> ---
>  block/iscsi.c |   80 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 files changed, 76 insertions(+), 4 deletions(-)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 95ce9e1..ac2ef1c 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -41,6 +41,7 @@
>  #include "qapi/qmp/qstring.h"
>  #include "crypto/secret.h"
>  
> +#include "qemu/uri.h"
>  #include <iscsi/iscsi.h>
>  #include <iscsi/scsi-lowlevel.h>
>  
> @@ -595,6 +596,18 @@ iscsi_co_writev_flags(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
>      iscsi_co_init_iscsitask(iscsilun, &iTask);
>  retry:
>      if (iscsilun->use_16_for_rw) {
> +#if LIBISCSI_API_VERSION >= (20160603)
> +        iTask.task = iscsi_write16_iov_task(iscsilun->iscsi, iscsilun->lun, lba,
> +                                            NULL, num_sectors * iscsilun->block_size,
> +                                            iscsilun->block_size, 0, 0, fua, 0, 0,
> +                                            iscsi_co_generic_cb, &iTask, (struct scsi_iovec *)iov->iov, iov->niov);
> +    } else {
> +        iTask.task = iscsi_write10_iov_task(iscsilun->iscsi, iscsilun->lun, lba,
> +                                            NULL, num_sectors * iscsilun->block_size,
> +                                            iscsilun->block_size, 0, 0, fua, 0, 0,
> +                                            iscsi_co_generic_cb, &iTask, (struct scsi_iovec *)iov->iov, iov->niov);
> +    }
> +#else
>          iTask.task = iscsi_write16_task(iscsilun->iscsi, iscsilun->lun, lba,
>                                          NULL, num_sectors * iscsilun->block_size,
>                                          iscsilun->block_size, 0, 0, fua, 0, 0,
> @@ -605,11 +618,14 @@ retry:
>                                          iscsilun->block_size, 0, 0, fua, 0, 0,
>                                          iscsi_co_generic_cb, &iTask);
>      }
> +#endif
>      if (iTask.task == NULL) {
>          return -ENOMEM;
>      }
> +#if LIBISCSI_API_VERSION < (20160603)
>      scsi_task_set_iov_out(iTask.task, (struct scsi_iovec *) iov->iov,
>                            iov->niov);
> +#endif
>      while (!iTask.complete) {
>          iscsi_set_events(iscsilun);
>          qemu_coroutine_yield();
> @@ -792,6 +808,19 @@ static int coroutine_fn iscsi_co_readv(BlockDriverState *bs,
>      iscsi_co_init_iscsitask(iscsilun, &iTask);
>  retry:
>      if (iscsilun->use_16_for_rw) {
> +#if LIBISCSI_API_VERSION >= (20160603)
> +        iTask.task = iscsi_read16_iov_task(iscsilun->iscsi, iscsilun->lun, lba,
> +                                           num_sectors * iscsilun->block_size,
> +                                           iscsilun->block_size, 0, 0, 0, 0, 0,
> +                                           iscsi_co_generic_cb, &iTask, (struct scsi_iovec *)iov->iov, iov->niov);
> +    } else {
> +        iTask.task = iscsi_read10_iov_task(iscsilun->iscsi, iscsilun->lun, lba,
> +                                           num_sectors * iscsilun->block_size,
> +                                           iscsilun->block_size,
> +                                           0, 0, 0, 0, 0,
> +                                           iscsi_co_generic_cb, &iTask, (struct scsi_iovec *)iov->iov, iov->niov);
> +    }
> +#else
>          iTask.task = iscsi_read16_task(iscsilun->iscsi, iscsilun->lun, lba,
>                                         num_sectors * iscsilun->block_size,
>                                         iscsilun->block_size, 0, 0, 0, 0, 0,
> @@ -803,11 +832,13 @@ retry:
>                                         0, 0, 0, 0, 0,
>                                         iscsi_co_generic_cb, &iTask);
>      }
> +#endif
>      if (iTask.task == NULL) {
>          return -ENOMEM;
>      }
> +#if LIBISCSI_API_VERSION < (20160603)
>      scsi_task_set_iov_in(iTask.task, (struct scsi_iovec *) iov->iov, iov->niov);
> -
> +#endif

These iov changes appear to be independent of iSER.  Please split them
into a separate patch.  Keeping logical changes in separate patches
makes it easier to review, bisect, backport, etc.

>      while (!iTask.complete) {
>          iscsi_set_events(iscsilun);
>          qemu_coroutine_yield();
> @@ -1592,9 +1623,9 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
>  
>      filename = qemu_opt_get(opts, "filename");
>  
> -    iscsi_url = iscsi_parse_full_url(iscsi, filename);
> +    iscsi_url = iscsi_parse_full_url(iscsi, uri_string_unescape(filename, -1, NULL));
>      if (iscsi_url == NULL) {
> -        error_setg(errp, "Failed to parse URL : %s", filename);
> +        error_setg(errp, "Failed to parse URL : %s", uri_string_unescape(filename, -1, NULL));

uri_string_unescape() returns a newly allocated string.  This is a
memory leak!

Is unescaping a bug fix?  Please put it into a separate patch.

>          ret = -EINVAL;
>          goto out;
>      }
> @@ -1609,7 +1640,13 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
>          ret = -ENOMEM;
>          goto out;
>      }
> -
> +#if LIBISCSI_API_VERSION >= (20160603)
> +    if (iscsi_init_transport(iscsi, iscsi_url->transport)) {
> +        error_setg(errp, ("Error initializing transport."));
> +        ret = -EINVAL;
> +        goto out;
> +    }
> +#endif
>      if (iscsi_set_targetname(iscsi, iscsi_url->target)) {
>          error_setg(errp, "iSCSI: Failed to set target name.");
>          ret = -EINVAL;
> @@ -2010,6 +2047,40 @@ static BlockDriver bdrv_iscsi = {
>      .bdrv_attach_aio_context = iscsi_attach_aio_context,
>  };
>  
> +static BlockDriver bdrv_iser = {
> +    .format_name     = "iser",
> +    .protocol_name   = "iser",
> +
> +    .instance_size   = sizeof(IscsiLun),
> +    .bdrv_needs_filename = true,
> +    .bdrv_file_open  = iscsi_open,
> +    .bdrv_close      = iscsi_close,
> +    .bdrv_create     = iscsi_create,
> +    .create_opts     = &iscsi_create_opts,
> +    .bdrv_reopen_prepare   = iscsi_reopen_prepare,
> +    .bdrv_reopen_commit    = iscsi_reopen_commit,
> +    .bdrv_invalidate_cache = iscsi_invalidate_cache,
> +
> +    .bdrv_getlength  = iscsi_getlength,
> +    .bdrv_get_info   = iscsi_get_info,
> +    .bdrv_truncate   = iscsi_truncate,
> +    .bdrv_refresh_limits = iscsi_refresh_limits,
> +
> +    .bdrv_co_get_block_status = iscsi_co_get_block_status,
> +    .bdrv_co_pdiscard      = iscsi_co_pdiscard,
> +    .bdrv_co_pwrite_zeroes = iscsi_co_pwrite_zeroes,
> +    .bdrv_co_readv         = iscsi_co_readv,
> +    .bdrv_co_writev_flags  = iscsi_co_writev_flags,
> +    .bdrv_co_flush_to_disk = iscsi_co_flush,
> +
> +#ifdef __linux__
> +    .bdrv_aio_ioctl   = iscsi_aio_ioctl,
> +#endif
> +
> +    .bdrv_detach_aio_context = iscsi_detach_aio_context,
> +    .bdrv_attach_aio_context = iscsi_attach_aio_context,
> +};

The commit description says ?iser needs to be added to the URI.  Why is
it necessary to define a new "iser" protocol block driver?

On 9/27/2016 1:28 PM, Stefan Hajnoczi wrote:
> On Wed, Sep 21, 2016 at 09:19:44PM +0300, Roy Shterman wrote:
>> iSER is a new transport layer supported in Libiscsi,
>> iSER provides a zero-copy RDMA capable interface that can
>> improve performance.
>>
>> New API is introduced in abstracion of the Libiscsi transport layer.
>> In order to use the new iSER transport, one need to add the ?iser option
>> at the end of Libiscsi URI.
>>
>> For now iSER memory buffers are pre-allocated and pre-registered,
>> hence in order to work with iSER from QEMU, one need to enable MEMLOCK
>> attribute in the VM to be large enough for all iSER buffers and RDMA
>> resources.
>>
>> A new functionallity is also introduced in this commit, a new API
>> to deploy zero-copy command submission. iSER is differing from TCP in
>> data-path, hence IO vectors must be transferred already when queueing
>> the PDU.
>>
>> changes from v1:
>> - Adding iser as an additional block driver
>>
>> Signed-off-by: Roy Shterman <roysh@mellanox.com>
>> ---
>>   block/iscsi.c |   80 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>>   1 files changed, 76 insertions(+), 4 deletions(-)
>>
>> diff --git a/block/iscsi.c b/block/iscsi.c
>> index 95ce9e1..ac2ef1c 100644
>> --- a/block/iscsi.c
>> +++ b/block/iscsi.c
>> @@ -41,6 +41,7 @@
>>   #include "qapi/qmp/qstring.h"
>>   #include "crypto/secret.h"
>>   
>> +#include "qemu/uri.h"
>>   #include <iscsi/iscsi.h>
>>   #include <iscsi/scsi-lowlevel.h>
>>   
>> @@ -595,6 +596,18 @@ iscsi_co_writev_flags(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
>>       iscsi_co_init_iscsitask(iscsilun, &iTask);
>>   retry:
>>       if (iscsilun->use_16_for_rw) {
>> +#if LIBISCSI_API_VERSION >= (20160603)
>> +        iTask.task = iscsi_write16_iov_task(iscsilun->iscsi, iscsilun->lun, lba,
>> +                                            NULL, num_sectors * iscsilun->block_size,
>> +                                            iscsilun->block_size, 0, 0, fua, 0, 0,
>> +                                            iscsi_co_generic_cb, &iTask, (struct scsi_iovec *)iov->iov, iov->niov);
>> +    } else {
>> +        iTask.task = iscsi_write10_iov_task(iscsilun->iscsi, iscsilun->lun, lba,
>> +                                            NULL, num_sectors * iscsilun->block_size,
>> +                                            iscsilun->block_size, 0, 0, fua, 0, 0,
>> +                                            iscsi_co_generic_cb, &iTask, (struct scsi_iovec *)iov->iov, iov->niov);
>> +    }
>> +#else
>>           iTask.task = iscsi_write16_task(iscsilun->iscsi, iscsilun->lun, lba,
>>                                           NULL, num_sectors * iscsilun->block_size,
>>                                           iscsilun->block_size, 0, 0, fua, 0, 0,
>> @@ -605,11 +618,14 @@ retry:
>>                                           iscsilun->block_size, 0, 0, fua, 0, 0,
>>                                           iscsi_co_generic_cb, &iTask);
>>       }
>> +#endif
>>       if (iTask.task == NULL) {
>>           return -ENOMEM;
>>       }
>> +#if LIBISCSI_API_VERSION < (20160603)
>>       scsi_task_set_iov_out(iTask.task, (struct scsi_iovec *) iov->iov,
>>                             iov->niov);
>> +#endif
>>       while (!iTask.complete) {
>>           iscsi_set_events(iscsilun);
>>           qemu_coroutine_yield();
>> @@ -792,6 +808,19 @@ static int coroutine_fn iscsi_co_readv(BlockDriverState *bs,
>>       iscsi_co_init_iscsitask(iscsilun, &iTask);
>>   retry:
>>       if (iscsilun->use_16_for_rw) {
>> +#if LIBISCSI_API_VERSION >= (20160603)
>> +        iTask.task = iscsi_read16_iov_task(iscsilun->iscsi, iscsilun->lun, lba,
>> +                                           num_sectors * iscsilun->block_size,
>> +                                           iscsilun->block_size, 0, 0, 0, 0, 0,
>> +                                           iscsi_co_generic_cb, &iTask, (struct scsi_iovec *)iov->iov, iov->niov);
>> +    } else {
>> +        iTask.task = iscsi_read10_iov_task(iscsilun->iscsi, iscsilun->lun, lba,
>> +                                           num_sectors * iscsilun->block_size,
>> +                                           iscsilun->block_size,
>> +                                           0, 0, 0, 0, 0,
>> +                                           iscsi_co_generic_cb, &iTask, (struct scsi_iovec *)iov->iov, iov->niov);
>> +    }
>> +#else
>>           iTask.task = iscsi_read16_task(iscsilun->iscsi, iscsilun->lun, lba,
>>                                          num_sectors * iscsilun->block_size,
>>                                          iscsilun->block_size, 0, 0, 0, 0, 0,
>> @@ -803,11 +832,13 @@ retry:
>>                                          0, 0, 0, 0, 0,
>>                                          iscsi_co_generic_cb, &iTask);
>>       }
>> +#endif
>>       if (iTask.task == NULL) {
>>           return -ENOMEM;
>>       }
>> +#if LIBISCSI_API_VERSION < (20160603)
>>       scsi_task_set_iov_in(iTask.task, (struct scsi_iovec *) iov->iov, iov->niov);
>> -
>> +#endif
> These iov changes appear to be independent of iSER.  Please split them
> into a separate patch.  Keeping logical changes in separate patches
> makes it easier to review, bisect, backport, etc.
I will split the patch into two different patches in v3 and resend it.
>>       while (!iTask.complete) {
>>           iscsi_set_events(iscsilun);
>>           qemu_coroutine_yield();
>> @@ -1592,9 +1623,9 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
>>   
>>       filename = qemu_opt_get(opts, "filename");
>>   
>> -    iscsi_url = iscsi_parse_full_url(iscsi, filename);
>> +    iscsi_url = iscsi_parse_full_url(iscsi, uri_string_unescape(filename, -1, NULL));
>>       if (iscsi_url == NULL) {
>> -        error_setg(errp, "Failed to parse URL : %s", filename);
>> +        error_setg(errp, "Failed to parse URL : %s", uri_string_unescape(filename, -1, NULL));
> uri_string_unescape() returns a newly allocated string.  This is a
> memory leak!
will be fixed in v3
>
> Is unescaping a bug fix?  Please put it into a separate patch.
because libvirt is parsing '?' char as %3F, I needed to parse to URI 
with unescaping.
>
>>           ret = -EINVAL;
>>           goto out;
>>       }
>> @@ -1609,7 +1640,13 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
>>           ret = -ENOMEM;
>>           goto out;
>>       }
>> -
>> +#if LIBISCSI_API_VERSION >= (20160603)
>> +    if (iscsi_init_transport(iscsi, iscsi_url->transport)) {
>> +        error_setg(errp, ("Error initializing transport."));
>> +        ret = -EINVAL;
>> +        goto out;
>> +    }
>> +#endif
>>       if (iscsi_set_targetname(iscsi, iscsi_url->target)) {
>>           error_setg(errp, "iSCSI: Failed to set target name.");
>>           ret = -EINVAL;
>> @@ -2010,6 +2047,40 @@ static BlockDriver bdrv_iscsi = {
>>       .bdrv_attach_aio_context = iscsi_attach_aio_context,
>>   };
>>   
>> +static BlockDriver bdrv_iser = {
>> +    .format_name     = "iser",
>> +    .protocol_name   = "iser",
>> +
>> +    .instance_size   = sizeof(IscsiLun),
>> +    .bdrv_needs_filename = true,
>> +    .bdrv_file_open  = iscsi_open,
>> +    .bdrv_close      = iscsi_close,
>> +    .bdrv_create     = iscsi_create,
>> +    .create_opts     = &iscsi_create_opts,
>> +    .bdrv_reopen_prepare   = iscsi_reopen_prepare,
>> +    .bdrv_reopen_commit    = iscsi_reopen_commit,
>> +    .bdrv_invalidate_cache = iscsi_invalidate_cache,
>> +
>> +    .bdrv_getlength  = iscsi_getlength,
>> +    .bdrv_get_info   = iscsi_get_info,
>> +    .bdrv_truncate   = iscsi_truncate,
>> +    .bdrv_refresh_limits = iscsi_refresh_limits,
>> +
>> +    .bdrv_co_get_block_status = iscsi_co_get_block_status,
>> +    .bdrv_co_pdiscard      = iscsi_co_pdiscard,
>> +    .bdrv_co_pwrite_zeroes = iscsi_co_pwrite_zeroes,
>> +    .bdrv_co_readv         = iscsi_co_readv,
>> +    .bdrv_co_writev_flags  = iscsi_co_writev_flags,
>> +    .bdrv_co_flush_to_disk = iscsi_co_flush,
>> +
>> +#ifdef __linux__
>> +    .bdrv_aio_ioctl   = iscsi_aio_ioctl,
>> +#endif
>> +
>> +    .bdrv_detach_aio_context = iscsi_detach_aio_context,
>> +    .bdrv_attach_aio_context = iscsi_attach_aio_context,
>> +};
> The commit description says ?iser needs to be added to the URI.  Why is
> it necessary to define a new "iser" protocol block driver?
PaoloB asked to add this option also, in Libiscsi the URI parser checks 
for 'iser://' as a different protocol or '?iser' as URI option.
I will add it into the commit message.

>

Am 27.09.2016 um 13:37 schrieb Roy Shterman:
>
>
> On 9/27/2016 1:28 PM, Stefan Hajnoczi wrote:
>> On Wed, Sep 21, 2016 at 09:19:44PM +0300, Roy Shterman wrote:
>>> iSER is a new transport layer supported in Libiscsi,
>>> iSER provides a zero-copy RDMA capable interface that can
>>> improve performance.
>>>
>>> New API is introduced in abstracion of the Libiscsi transport layer.
>>> In order to use the new iSER transport, one need to add the ?iser option
>>> at the end of Libiscsi URI.
>>>
>>> For now iSER memory buffers are pre-allocated and pre-registered,
>>> hence in order to work with iSER from QEMU, one need to enable MEMLOCK
>>> attribute in the VM to be large enough for all iSER buffers and RDMA
>>> resources.
>>>
>>> A new functionallity is also introduced in this commit, a new API
>>> to deploy zero-copy command submission. iSER is differing from TCP in
>>> data-path, hence IO vectors must be transferred already when queueing
>>> the PDU.
>>>
>>> changes from v1:
>>> - Adding iser as an additional block driver
>>>
>>> Signed-off-by: Roy Shterman <roysh@mellanox.com>
>>> ---
>>>   block/iscsi.c |   80 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>>>   1 files changed, 76 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/block/iscsi.c b/block/iscsi.c
>>> index 95ce9e1..ac2ef1c 100644
>>> --- a/block/iscsi.c
>>> +++ b/block/iscsi.c
>>> @@ -41,6 +41,7 @@
>>>   #include "qapi/qmp/qstring.h"
>>>   #include "crypto/secret.h"
>>>   +#include "qemu/uri.h"
>>>   #include <iscsi/iscsi.h>
>>>   #include <iscsi/scsi-lowlevel.h>
>>>   @@ -595,6 +596,18 @@ iscsi_co_writev_flags(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
>>>       iscsi_co_init_iscsitask(iscsilun, &iTask);
>>>   retry:
>>>       if (iscsilun->use_16_for_rw) {
>>> +#if LIBISCSI_API_VERSION >= (20160603)
>>> +        iTask.task = iscsi_write16_iov_task(iscsilun->iscsi, iscsilun->lun, lba,
>>> +                                            NULL, num_sectors * iscsilun->block_size,
>>> + iscsilun->block_size, 0, 0, fua, 0, 0,
>>> + iscsi_co_generic_cb, &iTask, (struct scsi_iovec *)iov->iov, iov->niov);
>>> +    } else {
>>> +        iTask.task = iscsi_write10_iov_task(iscsilun->iscsi, iscsilun->lun, lba,
>>> +                                            NULL, num_sectors * iscsilun->block_size,
>>> + iscsilun->block_size, 0, 0, fua, 0, 0,
>>> + iscsi_co_generic_cb, &iTask, (struct scsi_iovec *)iov->iov, iov->niov);
>>> +    }
>>> +#else
>>>           iTask.task = iscsi_write16_task(iscsilun->iscsi, iscsilun->lun, lba,
>>>                                           NULL, num_sectors * iscsilun->block_size,
>>> iscsilun->block_size, 0, 0, fua, 0, 0,
>>> @@ -605,11 +618,14 @@ retry:
>>> iscsilun->block_size, 0, 0, fua, 0, 0,
>>>                                           iscsi_co_generic_cb, &iTask);
>>>       }
>>> +#endif
>>>       if (iTask.task == NULL) {
>>>           return -ENOMEM;
>>>       }
>>> +#if LIBISCSI_API_VERSION < (20160603)
>>>       scsi_task_set_iov_out(iTask.task, (struct scsi_iovec *) iov->iov,
>>>                             iov->niov);
>>> +#endif
>>>       while (!iTask.complete) {
>>>           iscsi_set_events(iscsilun);
>>>           qemu_coroutine_yield();
>>> @@ -792,6 +808,19 @@ static int coroutine_fn iscsi_co_readv(BlockDriverState *bs,
>>>       iscsi_co_init_iscsitask(iscsilun, &iTask);
>>>   retry:
>>>       if (iscsilun->use_16_for_rw) {
>>> +#if LIBISCSI_API_VERSION >= (20160603)
>>> +        iTask.task = iscsi_read16_iov_task(iscsilun->iscsi, iscsilun->lun, lba,
>>> +                                           num_sectors * iscsilun->block_size,
>>> + iscsilun->block_size, 0, 0, 0, 0, 0,
>>> + iscsi_co_generic_cb, &iTask, (struct scsi_iovec *)iov->iov, iov->niov);
>>> +    } else {
>>> +        iTask.task = iscsi_read10_iov_task(iscsilun->iscsi, iscsilun->lun, lba,
>>> +                                           num_sectors * iscsilun->block_size,
>>> + iscsilun->block_size,
>>> +                                           0, 0, 0, 0, 0,
>>> + iscsi_co_generic_cb, &iTask, (struct scsi_iovec *)iov->iov, iov->niov);
>>> +    }
>>> +#else
>>>           iTask.task = iscsi_read16_task(iscsilun->iscsi, iscsilun->lun, lba,
>>>                                          num_sectors * iscsilun->block_size,
>>> iscsilun->block_size, 0, 0, 0, 0, 0,
>>> @@ -803,11 +832,13 @@ retry:
>>>                                          0, 0, 0, 0, 0,
>>>                                          iscsi_co_generic_cb, &iTask);
>>>       }
>>> +#endif
>>>       if (iTask.task == NULL) {
>>>           return -ENOMEM;
>>>       }
>>> +#if LIBISCSI_API_VERSION < (20160603)
>>>       scsi_task_set_iov_in(iTask.task, (struct scsi_iovec *) iov->iov, iov->niov);
>>> -
>>> +#endif
>> These iov changes appear to be independent of iSER.  Please split them
>> into a separate patch.  Keeping logical changes in separate patches
>> makes it easier to review, bisect, backport, etc.
> I will split the patch into two different patches in v3 and resend it.
>>>       while (!iTask.complete) {
>>>           iscsi_set_events(iscsilun);
>>>           qemu_coroutine_yield();
>>> @@ -1592,9 +1623,9 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
>>>         filename = qemu_opt_get(opts, "filename");
>>>   -    iscsi_url = iscsi_parse_full_url(iscsi, filename);
>>> +    iscsi_url = iscsi_parse_full_url(iscsi, uri_string_unescape(filename, -1, NULL));
>>>       if (iscsi_url == NULL) {
>>> -        error_setg(errp, "Failed to parse URL : %s", filename);
>>> +        error_setg(errp, "Failed to parse URL : %s", uri_string_unescape(filename, -1, NULL));
>> uri_string_unescape() returns a newly allocated string.  This is a
>> memory leak!
> will be fixed in v3
>>
>> Is unescaping a bug fix?  Please put it into a separate patch.
> because libvirt is parsing '?' char as %3F, I needed to parse to URI with unescaping.
>>
>>>           ret = -EINVAL;
>>>           goto out;
>>>       }
>>> @@ -1609,7 +1640,13 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
>>>           ret = -ENOMEM;
>>>           goto out;
>>>       }
>>> -
>>> +#if LIBISCSI_API_VERSION >= (20160603)
>>> +    if (iscsi_init_transport(iscsi, iscsi_url->transport)) {
>>> +        error_setg(errp, ("Error initializing transport."));
>>> +        ret = -EINVAL;
>>> +        goto out;
>>> +    }
>>> +#endif
>>>       if (iscsi_set_targetname(iscsi, iscsi_url->target)) {
>>>           error_setg(errp, "iSCSI: Failed to set target name.");
>>>           ret = -EINVAL;
>>> @@ -2010,6 +2047,40 @@ static BlockDriver bdrv_iscsi = {
>>>       .bdrv_attach_aio_context = iscsi_attach_aio_context,
>>>   };
>>>   +static BlockDriver bdrv_iser = {
>>> +    .format_name     = "iser",
>>> +    .protocol_name   = "iser",
>>> +
>>> +    .instance_size   = sizeof(IscsiLun),
>>> +    .bdrv_needs_filename = true,
>>> +    .bdrv_file_open  = iscsi_open,
>>> +    .bdrv_close      = iscsi_close,
>>> +    .bdrv_create     = iscsi_create,
>>> +    .create_opts     = &iscsi_create_opts,
>>> +    .bdrv_reopen_prepare   = iscsi_reopen_prepare,
>>> +    .bdrv_reopen_commit    = iscsi_reopen_commit,
>>> +    .bdrv_invalidate_cache = iscsi_invalidate_cache,
>>> +
>>> +    .bdrv_getlength  = iscsi_getlength,
>>> +    .bdrv_get_info   = iscsi_get_info,
>>> +    .bdrv_truncate   = iscsi_truncate,
>>> +    .bdrv_refresh_limits = iscsi_refresh_limits,
>>> +
>>> +    .bdrv_co_get_block_status = iscsi_co_get_block_status,
>>> +    .bdrv_co_pdiscard      = iscsi_co_pdiscard,
>>> +    .bdrv_co_pwrite_zeroes = iscsi_co_pwrite_zeroes,
>>> +    .bdrv_co_readv         = iscsi_co_readv,
>>> +    .bdrv_co_writev_flags  = iscsi_co_writev_flags,
>>> +    .bdrv_co_flush_to_disk = iscsi_co_flush,
>>> +
>>> +#ifdef __linux__
>>> +    .bdrv_aio_ioctl   = iscsi_aio_ioctl,
>>> +#endif
>>> +
>>> +    .bdrv_detach_aio_context = iscsi_detach_aio_context,
>>> +    .bdrv_attach_aio_context = iscsi_attach_aio_context,
>>> +};
>> The commit description says ?iser needs to be added to the URI. Why is
>> it necessary to define a new "iser" protocol block driver?
> PaoloB asked to add this option also, in Libiscsi the URI parser checks for 'iser://' as a different protocol or '?iser' as URI option.
> I will add it into the commit message.
>
>>
>

When you send a v3 please check your patches with scripts/checkpatch.pl for style errors.

Thanks,
Peter

On 27/09/2016 13:37, Roy Shterman wrote:
>>>
>>> +    iscsi_url = iscsi_parse_full_url(iscsi,
>>> uri_string_unescape(filename, -1, NULL));
>>>       if (iscsi_url == NULL) {
>>> -        error_setg(errp, "Failed to parse URL : %s", filename);
>>> +        error_setg(errp, "Failed to parse URL : %s",
>>> uri_string_unescape(filename, -1, NULL));
>> uri_string_unescape() returns a newly allocated string.  This is a
>> memory leak!
> will be fixed in v3
>>
>> Is unescaping a bug fix?  Please put it into a separate patch.
> because libvirt is parsing '?' char as %3F, I needed to parse to URI
> with unescaping.

This looks like a libvirt bug.  But if libvirt learns to pass iser://
URIs, the unescape is not necessary, is it?

Thanks,

Paolo

On 9/27/2016 2:52 PM, Paolo Bonzini wrote:
>
> On 27/09/2016 13:37, Roy Shterman wrote:
>>>> +    iscsi_url = iscsi_parse_full_url(iscsi,
>>>> uri_string_unescape(filename, -1, NULL));
>>>>        if (iscsi_url == NULL) {
>>>> -        error_setg(errp, "Failed to parse URL : %s", filename);
>>>> +        error_setg(errp, "Failed to parse URL : %s",
>>>> uri_string_unescape(filename, -1, NULL));
>>> uri_string_unescape() returns a newly allocated string.  This is a
>>> memory leak!
>> will be fixed in v3
>>> Is unescaping a bug fix?  Please put it into a separate patch.
>> because libvirt is parsing '?' char as %3F, I needed to parse to URI
>> with unescaping.
> This looks like a libvirt bug.  But if libvirt learns to pass iser://
> URIs, the unescape is not necessary, is it?
right, but because libvirt inbox versions doesn't support 
protocol_name=iser I decided to leave both options available.
The ?iser option and the iser:// option, to also have compatibility with 
inbox Libvirt versions.

> Thanks,
>
> Paolo

> On 9/27/2016 2:52 PM, Paolo Bonzini wrote:
> > On 27/09/2016 13:37, Roy Shterman wrote:
> > > > > +    iscsi_url = iscsi_parse_full_url(iscsi,
> > > > >  uri_string_unescape(filename, -1, NULL));
> > > > >        if (iscsi_url == NULL) {
> > > > > -        error_setg(errp, "Failed to parse URL : %s", filename);
> > > > > +        error_setg(errp, "Failed to parse URL : %s",
> > > > > uri_string_unescape(filename, -1, NULL));
> > > >
> > > > uri_string_unescape() returns a newly allocated string.  This is a
> > > > memory leak!
> > > > Is unescaping a bug fix?  Please put it into a separate patch.
> > >
> > > because libvirt is parsing '?' char as %3F, I needed to parse to URI
> > > with unescaping.
> >
> > This looks like a libvirt bug.  But if libvirt learns to pass iser://
> > URIs, the unescape is not necessary, is it?
>
> right, but because libvirt inbox versions doesn't support
> protocol_name=iser I decided to leave both options available.
> The ?iser option and the iser:// option, to also have compatibility with
> inbox Libvirt versions.

No, please don't do that, especially because unescaping the URI is wrong:
if somebody puts a %2F in the libvirt XML it should _not_ separate the IQN
from the LUN number for example.

Paolo