[Qemu-devel] [PATCH] iSCSI: start moving options also for -drive

[Qemu-devel] [PATCH] iSCSI: start moving options also for -drive

[Pino Toscano]

Hi,

to overcome the limitations of the options handling [1], I'm planning
to move more options for iSCSI also as block options, so it is possible
to specify them with -drive.

The only patch in this series is for initiator-target, as I want to be
sure the approach is correct, and wanted.

[1] http://lists.nongnu.org/archive/html/qemu-devel/2015-11/msg06501.html

Thanks,

Pino Toscano (1):
  iscsi: allow "initiator-name" as block option

 block/iscsi.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

-- 
2.5.5

[Qemu-devel] [PATCH] iscsi: allow "initiator-name" as block option

[Pino Toscano]

Allow the "initiator-name" for both the -iscsi and the block options:
this way it is possible to set it directly as option in the -drive
specification.
The current way to specify the initiator name for a certain iSCSI
target is:
  -iscsi id=TARGET,initiator-name=IQN
which cannot be actually done when TARGET has the optional part, as
colon is not accepted as id for QemuOpts [1].

Hence, allow the "initiator-name" also in block options: this way
it is possible to set it directly as option in -drive, e.g.:
  -drive file=URI,driver=iscsi,initiator-name=IQN

[1] http://lists.nongnu.org/archive/html/qemu-devel/2015-11/msg06501.html

Signed-off-by: Pino Toscano <ptoscano@redhat.com>
---
 block/iscsi.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 302baf8..4a1c300 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1161,7 +1161,7 @@ static void parse_header_digest(struct iscsi_context *iscsi, const char *target,
     }
 }
 
-static char *parse_initiator_name(const char *target)
+static char *parse_initiator_name(QDict *options, const char *target)
 {
     QemuOptsList *list;
     QemuOpts *opts;
@@ -1169,6 +1169,11 @@ static char *parse_initiator_name(const char *target)
     char *iscsi_name;
     UuidInfo *uuid_info;
 
+    name = qdict_get_try_str(options, "initiator-name");
+    if (name != NULL) {
+        return g_strdup(name);
+    }
+
     list = qemu_find_opts("iscsi");
     if (list) {
         opts = qemu_opts_find(list, target);
@@ -1304,11 +1309,19 @@ static void iscsi_readcapacity_sync(IscsiLun *iscsilun, Error **errp)
     }
 }
 
+#define COMMON_ISCSI_OPTS \
+    { \
+        .name = "initiator-name", \
+        .type = QEMU_OPT_STRING, \
+        .help = "Initiator iqn name to use when connecting", \
+    }
+
 /* TODO Convert to fine grained options */
 static QemuOptsList runtime_opts = {
     .name = "iscsi",
     .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
     .desc = {
+        COMMON_ISCSI_OPTS,
         {
             .name = "filename",
             .type = QEMU_OPT_STRING,
@@ -1473,7 +1486,7 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
 
     memset(iscsilun, 0, sizeof(IscsiLun));
 
-    initiator_name = parse_initiator_name(iscsi_url->target);
+    initiator_name = parse_initiator_name(bs->options, iscsi_url->target);
 
     iscsi = iscsi_create_context(initiator_name);
     if (iscsi == NULL) {
@@ -1864,6 +1877,7 @@ static QemuOptsList qemu_iscsi_opts = {
     .name = "iscsi",
     .head = QTAILQ_HEAD_INITIALIZER(qemu_iscsi_opts.head),
     .desc = {
+        COMMON_ISCSI_OPTS,
         {
             .name = "user",
             .type = QEMU_OPT_STRING,
@@ -1883,10 +1897,6 @@ static QemuOptsList qemu_iscsi_opts = {
             .help = "HeaderDigest setting. "
                     "{CRC32C|CRC32C-NONE|NONE-CRC32C|NONE}",
         },{
-            .name = "initiator-name",
-            .type = QEMU_OPT_STRING,
-            .help = "Initiator iqn name to use when connecting",
-        },{
             .name = "timeout",
             .type = QEMU_OPT_NUMBER,
             .help = "Request timeout in seconds (default 0 = no timeout)",
-- 
2.5.5

Re: [Qemu-devel] [PATCH] iscsi: allow "initiator-name" as block option

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3945 bytes --]

On 12.04.2016 16:57, Pino Toscano wrote:
> Allow the "initiator-name" for both the -iscsi and the block options:
> this way it is possible to set it directly as option in the -drive
> specification.
> The current way to specify the initiator name for a certain iSCSI
> target is:
>   -iscsi id=TARGET,initiator-name=IQN
> which cannot be actually done when TARGET has the optional part, as
> colon is not accepted as id for QemuOpts [1].
> 
> Hence, allow the "initiator-name" also in block options: this way
> it is possible to set it directly as option in -drive, e.g.:
>   -drive file=URI,driver=iscsi,initiator-name=IQN
> 
> [1] http://lists.nongnu.org/archive/html/qemu-devel/2015-11/msg06501.html
> 
> Signed-off-by: Pino Toscano <ptoscano@redhat.com>
> ---
>  block/iscsi.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 302baf8..4a1c300 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -1161,7 +1161,7 @@ static void parse_header_digest(struct iscsi_context *iscsi, const char *target,
>      }
>  }
>  
> -static char *parse_initiator_name(const char *target)
> +static char *parse_initiator_name(QDict *options, const char *target)
>  {
>      QemuOptsList *list;
>      QemuOpts *opts;
> @@ -1169,6 +1169,11 @@ static char *parse_initiator_name(const char *target)
>      char *iscsi_name;
>      UuidInfo *uuid_info;
>  
> +    name = qdict_get_try_str(options, "initiator-name");
> +    if (name != NULL) {
> +        return g_strdup(name);
> +    }

You should not be using the "options" QDict here but the already parsed
QemuOpts ("opts" in the caller). That is, the caller should either pass
said @opts or use qemu_opt_get(opts, "initiator-name") to get the name
and pass that then.

> +
>      list = qemu_find_opts("iscsi");
>      if (list) {
>          opts = qemu_opts_find(list, target);
> @@ -1304,11 +1309,19 @@ static void iscsi_readcapacity_sync(IscsiLun *iscsilun, Error **errp)
>      }
>  }
>  
> +#define COMMON_ISCSI_OPTS \
> +    { \
> +        .name = "initiator-name", \
> +        .type = QEMU_OPT_STRING, \
> +        .help = "Initiator iqn name to use when connecting", \
> +    }
> +
>  /* TODO Convert to fine grained options */
>  static QemuOptsList runtime_opts = {
>      .name = "iscsi",
>      .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
>      .desc = {
> +        COMMON_ISCSI_OPTS,
>          {
>              .name = "filename",
>              .type = QEMU_OPT_STRING,
> @@ -1473,7 +1486,7 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
>  
>      memset(iscsilun, 0, sizeof(IscsiLun));
>  
> -    initiator_name = parse_initiator_name(iscsi_url->target);
> +    initiator_name = parse_initiator_name(bs->options, iscsi_url->target);
>  
>      iscsi = iscsi_create_context(initiator_name);
>      if (iscsi == NULL) {
> @@ -1864,6 +1877,7 @@ static QemuOptsList qemu_iscsi_opts = {
>      .name = "iscsi",
>      .head = QTAILQ_HEAD_INITIALIZER(qemu_iscsi_opts.head),
>      .desc = {
> +        COMMON_ISCSI_OPTS,
>          {
>              .name = "user",
>              .type = QEMU_OPT_STRING,
> @@ -1883,10 +1897,6 @@ static QemuOptsList qemu_iscsi_opts = {
>              .help = "HeaderDigest setting. "
>                      "{CRC32C|CRC32C-NONE|NONE-CRC32C|NONE}",
>          },{
> -            .name = "initiator-name",
> -            .type = QEMU_OPT_STRING,
> -            .help = "Initiator iqn name to use when connecting",
> -        },{
>              .name = "timeout",
>              .type = QEMU_OPT_NUMBER,
>              .help = "Request timeout in seconds (default 0 = no timeout)",
> 

This needs to be rebased on block-next (which will be merged to master
once the 2.8 window is open), because these options are now in vl.c.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]

Re: [Qemu-devel] [PATCH] iSCSI: start moving options also for -drive

[Daniel P. Berrange]

On Tue, Apr 12, 2016 at 04:57:42PM +0200, Pino Toscano wrote:
> Hi,
> 
> to overcome the limitations of the options handling [1], I'm planning
> to move more options for iSCSI also as block options, so it is possible
> to specify them with -drive.
> 
> The only patch in this series is for initiator-target, as I want to be
> sure the approach is correct, and wanted.

I think the approach makes sense - the current -iscsi arg is utter
madness as a concept, so the sooner everything is allowed via
-drive the better

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH] iSCSI: start moving options also for -drive

[Pino Toscano]

[-- Attachment #1: Type: text/plain, Size: 654 bytes --]

On Tuesday, 12 April 2016 16:57:42 CEST Pino Toscano wrote:
> to overcome the limitations of the options handling [1], I'm planning
> to move more options for iSCSI also as block options, so it is possible
> to specify them with -drive.
> 
> The only patch in this series is for initiator-target, as I want to be
> sure the approach is correct, and wanted.
> 
> [1] http://lists.nongnu.org/archive/html/qemu-devel/2015-11/msg06501.html
> 
> Thanks,
> 
> Pino Toscano (1):
>   iscsi: allow "initiator-name" as block option
> 
>  block/iscsi.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)

Ping.

Thanks,
-- 
Pino Toscano

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH] iSCSI: start moving options also for -drive

[Paolo Bonzini]

On 17/08/2016 14:13, Pino Toscano wrote:
> On Tuesday, 12 April 2016 16:57:42 CEST Pino Toscano wrote:
>> to overcome the limitations of the options handling [1], I'm planning
>> to move more options for iSCSI also as block options, so it is possible
>> to specify them with -drive.
>>
>> The only patch in this series is for initiator-target, as I want to be
>> sure the approach is correct, and wanted.
>>
>> [1] http://lists.nongnu.org/archive/html/qemu-devel/2015-11/msg06501.html
>>
>> Thanks,
>>
>> Pino Toscano (1):
>>   iscsi: allow "initiator-name" as block option
>>
>>  block/iscsi.c | 22 ++++++++++++++++------
>>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> Ping.
> 
> Thanks,
> 

I think this makes sense, but Kevin or Max should review it.

Paolo