[Qemu-devel] [RFC PATCH 0/6] iscsi: Add blockdev-add support

[Qemu-devel] [RFC PATCH 0/6] iscsi: Add blockdev-add support

[Kevin Wolf]

This adds blockdev-add support to the iscsi block driver.

Note that this is only compile tested at this point. Jeff is going to
take over from here and bring the series to a mergable state.

Kevin Wolf (6):
  iscsi: Split URL into individual options
  iscsi: Handle -iscsi user/password in bdrv_parse_filename()
  iscsi: Add initiator-name option
  iscsi: Add header-digest option
  iscsi: Add timeout option
  iscsi: Add blockdev-add support

 block/iscsi.c        | 342 ++++++++++++++++++++++++++++++---------------------
 qapi/block-core.json |  74 ++++++++++-
 2 files changed, 272 insertions(+), 144 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [RFC PATCH 1/6] iscsi: Split URL into individual options

[Kevin Wolf]

This introduces a .bdrv_parse_filename handler for iscsi which parses an
URL if given and translates it to individual options.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/iscsi.c | 189 ++++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 136 insertions(+), 53 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 0960929..7a6664e 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1469,20 +1469,6 @@ static void iscsi_readcapacity_sync(IscsiLun *iscsilun, Error **errp)
     }
 }
 
-/* TODO Convert to fine grained options */
-static QemuOptsList runtime_opts = {
-    .name = "iscsi",
-    .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
-    .desc = {
-        {
-            .name = "filename",
-            .type = QEMU_OPT_STRING,
-            .help = "URL to the iscsi image",
-        },
-        { /* end of list */ }
-    },
-};
-
 static struct scsi_task *iscsi_do_inquiry(struct iscsi_context *iscsi, int lun,
                                           int evpd, int pc, void **inq, Error **errp)
 {
@@ -1604,20 +1590,98 @@ out:
  * We support iscsi url's on the form
  * iscsi://[<username>%<password>@]<host>[:<port>]/<targetname>/<lun>
  */
+static void iscsi_parse_filename(const char *filename, QDict *options,
+                                 Error **errp)
+{
+    struct iscsi_url *iscsi_url;
+    const char *transport_name;
+    char *lun_str;
+
+    iscsi_url = iscsi_parse_full_url(NULL, filename);
+    if (iscsi_url == NULL) {
+        error_setg(errp, "Failed to parse URL : %s", filename);
+        return;
+    }
+
+#if LIBISCSI_API_VERSION >= (20160603)
+    switch (iscsi_url->transport) {
+        case TCP_TRANSPORT:     transport_name = "tcp"; break;
+        case ISER_TRANSPORT:    transport_name = "iser"; break;
+        default:
+            error_setg(errp, "Unknown transport type (%d)",
+                       iscsi_url->transport);
+            return;
+    }
+#else
+    transport_name = "tcp";
+#endif
+
+    qdict_set_default_str(options, "transport", transport_name);
+    qdict_set_default_str(options, "portal", iscsi_url->portal);
+    qdict_set_default_str(options, "target", iscsi_url->target);
+
+    lun_str = g_strdup_printf("%d", iscsi_url->lun);
+    qdict_set_default_str(options, "lun", lun_str);
+    g_free(lun_str);
+
+    if (iscsi_url->user[0] != '\0') {
+        qdict_set_default_str(options, "user", iscsi_url->user);
+        qdict_set_default_str(options, "password", iscsi_url->passwd);
+    }
+
+    iscsi_destroy_url(iscsi_url);
+}
+
+/* TODO Add -iscsi options */
+static QemuOptsList runtime_opts = {
+    .name = "iscsi",
+    .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
+    .desc = {
+        {
+            .name = "transport",
+            .type = QEMU_OPT_STRING,
+        },
+        {
+            .name = "portal",
+            .type = QEMU_OPT_STRING,
+        },
+        {
+            .name = "target",
+            .type = QEMU_OPT_STRING,
+        },
+        {
+            .name = "user",
+            .type = QEMU_OPT_STRING,
+        },
+        {
+            .name = "password",
+            .type = QEMU_OPT_STRING,
+        },
+        {
+            .name = "lun",
+            .type = QEMU_OPT_NUMBER,
+        },
+        { /* end of list */ }
+    },
+};
+
 static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
                       Error **errp)
 {
     IscsiLun *iscsilun = bs->opaque;
     struct iscsi_context *iscsi = NULL;
-    struct iscsi_url *iscsi_url = NULL;
     struct scsi_task *task = NULL;
     struct scsi_inquiry_standard *inq = NULL;
     struct scsi_inquiry_supported_pages *inq_vpd;
     char *initiator_name = NULL;
     QemuOpts *opts;
     Error *local_err = NULL;
-    const char *filename;
-    int i, ret = 0, timeout = 0;
+    const char *transport_name, *portal, *target;
+    const char *user, *password;
+#if LIBISCSI_API_VERSION >= (20160603)
+    enum iscsi_transport_type transport;
+#endif
+    int i, ret = 0, timeout = 0, lun;
 
     opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
     qemu_opts_absorb_qdict(opts, options, &local_err);
@@ -1627,18 +1691,41 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
         goto out;
     }
 
-    filename = qemu_opt_get(opts, "filename");
+    transport_name = qemu_opt_get(opts, "transport");
+    portal = qemu_opt_get(opts, "portal");
+    target = qemu_opt_get(opts, "target");
+    user = qemu_opt_get(opts, "user");
+    password = qemu_opt_get(opts, "password");
+    lun = qemu_opt_get_number(opts, "lun", 0);
 
-    iscsi_url = iscsi_parse_full_url(iscsi, filename);
-    if (iscsi_url == NULL) {
-        error_setg(errp, "Failed to parse URL : %s", filename);
+    if (!transport_name || !portal || !target) {
+        error_setg(errp, "Need all of transport, portal and target options");
+        ret = -EINVAL;
+        goto out;
+    }
+    if (user && !password) {
+        error_setg(errp, "If a user name is given, a password is required");
+        ret = -EINVAL;
+        goto out;
+    }
+
+    if (!strcmp(transport_name, "tcp")) {
+#if LIBISCSI_API_VERSION >= (20160603)
+        transport = TCP_TRANSPORT;
+    } else if (!strcmp(transport_name, "iser")) {
+        transport = ISER_TRANSPORT;
+#else
+        /* TCP is what older libiscsi versions always use */
+#endif
+    } else {
+        error_setg(errp, "Unknown transport: %s", transport_name);
         ret = -EINVAL;
         goto out;
     }
 
     memset(iscsilun, 0, sizeof(IscsiLun));
 
-    initiator_name = parse_initiator_name(iscsi_url->target);
+    initiator_name = parse_initiator_name(target);
 
     iscsi = iscsi_create_context(initiator_name);
     if (iscsi == NULL) {
@@ -1647,21 +1734,20 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
         goto out;
     }
 #if LIBISCSI_API_VERSION >= (20160603)
-    if (iscsi_init_transport(iscsi, iscsi_url->transport)) {
+    if (iscsi_init_transport(iscsi, transport)) {
         error_setg(errp, ("Error initializing transport."));
         ret = -EINVAL;
         goto out;
     }
 #endif
-    if (iscsi_set_targetname(iscsi, iscsi_url->target)) {
+    if (iscsi_set_targetname(iscsi, target)) {
         error_setg(errp, "iSCSI: Failed to set target name.");
         ret = -EINVAL;
         goto out;
     }
 
-    if (iscsi_url->user[0] != '\0') {
-        ret = iscsi_set_initiator_username_pwd(iscsi, iscsi_url->user,
-                                              iscsi_url->passwd);
+    if (user) {
+        ret = iscsi_set_initiator_username_pwd(iscsi, user, password);
         if (ret != 0) {
             error_setg(errp, "Failed to set initiator username and password");
             ret = -EINVAL;
@@ -1670,7 +1756,7 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     /* check if we got CHAP username/password via the options */
-    parse_chap(iscsi, iscsi_url->target, &local_err);
+    parse_chap(iscsi, target, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
         ret = -EINVAL;
@@ -1686,7 +1772,7 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     iscsi_set_header_digest(iscsi, ISCSI_HEADER_DIGEST_NONE_CRC32C);
 
     /* check if we got HEADER_DIGEST via the options */
-    parse_header_digest(iscsi, iscsi_url->target, &local_err);
+    parse_header_digest(iscsi, target, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
         ret = -EINVAL;
@@ -1694,7 +1780,7 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     /* timeout handling is broken in libiscsi before 1.15.0 */
-    timeout = parse_timeout(iscsi_url->target);
+    timeout = parse_timeout(target);
 #if LIBISCSI_API_VERSION >= 20150621
     iscsi_set_timeout(iscsi, timeout);
 #else
@@ -1703,7 +1789,7 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     }
 #endif
 
-    if (iscsi_full_connect_sync(iscsi, iscsi_url->portal, iscsi_url->lun) != 0) {
+    if (iscsi_full_connect_sync(iscsi, portal, lun) != 0) {
         error_setg(errp, "iSCSI: Failed to connect to LUN : %s",
             iscsi_get_error(iscsi));
         ret = -EINVAL;
@@ -1712,7 +1798,7 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
 
     iscsilun->iscsi = iscsi;
     iscsilun->aio_context = bdrv_get_aio_context(bs);
-    iscsilun->lun   = iscsi_url->lun;
+    iscsilun->lun = lun;
     iscsilun->has_write_same = true;
 
     task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 0, 0,
@@ -1815,9 +1901,6 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
 out:
     qemu_opts_del(opts);
     g_free(initiator_name);
-    if (iscsi_url != NULL) {
-        iscsi_destroy_url(iscsi_url);
-    }
     if (task != NULL) {
         scsi_free_scsi_task(task);
     }
@@ -2026,15 +2109,15 @@ static BlockDriver bdrv_iscsi = {
     .format_name     = "iscsi",
     .protocol_name   = "iscsi",
 
-    .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,
+    .instance_size          = sizeof(IscsiLun),
+    .bdrv_parse_filename    = iscsi_parse_filename,
+    .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,
@@ -2061,15 +2144,15 @@ 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,
+    .instance_size          = sizeof(IscsiLun),
+    .bdrv_parse_filename    = iscsi_parse_filename,
+    .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,
-- 
1.8.3.1

Re: [Qemu-devel] [RFC PATCH 1/6] iscsi: Split URL into individual options

[Daniel P. Berrange]

On Thu, Dec 08, 2016 at 02:23:06PM +0100, Kevin Wolf wrote:
> This introduces a .bdrv_parse_filename handler for iscsi which parses an
> URL if given and translates it to individual options.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/iscsi.c | 189 ++++++++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 136 insertions(+), 53 deletions(-)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 0960929..7a6664e 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -1469,20 +1469,6 @@ static void iscsi_readcapacity_sync(IscsiLun *iscsilun, Error **errp)
>      }
>  }
>  
> -/* TODO Convert to fine grained options */
> -static QemuOptsList runtime_opts = {
> -    .name = "iscsi",
> -    .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
> -    .desc = {
> -        {
> -            .name = "filename",
> -            .type = QEMU_OPT_STRING,
> -            .help = "URL to the iscsi image",
> -        },
> -        { /* end of list */ }
> -    },
> -};
> -
>  static struct scsi_task *iscsi_do_inquiry(struct iscsi_context *iscsi, int lun,
>                                            int evpd, int pc, void **inq, Error **errp)
>  {
> @@ -1604,20 +1590,98 @@ out:
>   * We support iscsi url's on the form
>   * iscsi://[<username>%<password>@]<host>[:<port>]/<targetname>/<lun>
>   */
> +static void iscsi_parse_filename(const char *filename, QDict *options,
> +                                 Error **errp)
> +{
> +    struct iscsi_url *iscsi_url;
> +    const char *transport_name;
> +    char *lun_str;
> +
> +    iscsi_url = iscsi_parse_full_url(NULL, filename);
> +    if (iscsi_url == NULL) {
> +        error_setg(errp, "Failed to parse URL : %s", filename);
> +        return;
> +    }
> +
> +#if LIBISCSI_API_VERSION >= (20160603)
> +    switch (iscsi_url->transport) {
> +        case TCP_TRANSPORT:     transport_name = "tcp"; break;
> +        case ISER_TRANSPORT:    transport_name = "iser"; break;
> +        default:
> +            error_setg(errp, "Unknown transport type (%d)",
> +                       iscsi_url->transport);
> +            return;
> +    }
> +#else
> +    transport_name = "tcp";
> +#endif

Here if the URI contained a transport "iser" and we're using
an old libiscsi, we silently ignore that and use 'tcp'. IMHO
we should be reporting "Unsupported transport type 'iser'"
when using the old libiscsi API/


> +    qdict_set_default_str(options, "transport", transport_name);
> +    qdict_set_default_str(options, "portal", iscsi_url->portal);
> +    qdict_set_default_str(options, "target", iscsi_url->target);
> +
> +    lun_str = g_strdup_printf("%d", iscsi_url->lun);
> +    qdict_set_default_str(options, "lun", lun_str);
> +    g_free(lun_str);
> +
> +    if (iscsi_url->user[0] != '\0') {
> +        qdict_set_default_str(options, "user", iscsi_url->user);
> +        qdict_set_default_str(options, "password", iscsi_url->passwd);

If the URI contains "user" but not "password", this stores bogus
data in the "password" field I believe.

> +    }
> +
> +    iscsi_destroy_url(iscsi_url);
> +}
> +
> +/* TODO Add -iscsi options */
> +static QemuOptsList runtime_opts = {
> +    .name = "iscsi",
> +    .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
> +    .desc = {
> +        {
> +            .name = "transport",
> +            .type = QEMU_OPT_STRING,
> +        },
> +        {
> +            .name = "portal",
> +            .type = QEMU_OPT_STRING,
> +        },
> +        {
> +            .name = "target",
> +            .type = QEMU_OPT_STRING,
> +        },
> +        {
> +            .name = "user",
> +            .type = QEMU_OPT_STRING,
> +        },
> +        {
> +            .name = "password",
> +            .type = QEMU_OPT_STRING,
> +        },
> +        {
> +            .name = "lun",
> +            .type = QEMU_OPT_NUMBER,
> +        },
> +        { /* end of list */ }
> +    },
> +};
> +
>  static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
>                        Error **errp)
>  {
>      IscsiLun *iscsilun = bs->opaque;
>      struct iscsi_context *iscsi = NULL;
> -    struct iscsi_url *iscsi_url = NULL;
>      struct scsi_task *task = NULL;
>      struct scsi_inquiry_standard *inq = NULL;
>      struct scsi_inquiry_supported_pages *inq_vpd;
>      char *initiator_name = NULL;
>      QemuOpts *opts;
>      Error *local_err = NULL;
> -    const char *filename;
> -    int i, ret = 0, timeout = 0;
> +    const char *transport_name, *portal, *target;
> +    const char *user, *password;
> +#if LIBISCSI_API_VERSION >= (20160603)
> +    enum iscsi_transport_type transport;
> +#endif
> +    int i, ret = 0, timeout = 0, lun;
>  
>      opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
>      qemu_opts_absorb_qdict(opts, options, &local_err);
> @@ -1627,18 +1691,41 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
>          goto out;
>      }
>  
> -    filename = qemu_opt_get(opts, "filename");
> +    transport_name = qemu_opt_get(opts, "transport");
> +    portal = qemu_opt_get(opts, "portal");
> +    target = qemu_opt_get(opts, "target");
> +    user = qemu_opt_get(opts, "user");
> +    password = qemu_opt_get(opts, "password");
> +    lun = qemu_opt_get_number(opts, "lun", 0);

Do we really want to default to '0' for LUN ?  With Linux tgtd at
least this is not a valid LUN to use as a volume. It feels like
we should be raising an error if 'lun' is not explcitly set by
the user

>  
> -    iscsi_url = iscsi_parse_full_url(iscsi, filename);
> -    if (iscsi_url == NULL) {
> -        error_setg(errp, "Failed to parse URL : %s", filename);
> +    if (!transport_name || !portal || !target) {
> +        error_setg(errp, "Need all of transport, portal and target options");
> +        ret = -EINVAL;
> +        goto out;
> +    }
> +    if (user && !password) {
> +        error_setg(errp, "If a user name is given, a password is required");
> +        ret = -EINVAL;
> +        goto out;
> +    }
> +
> +    if (!strcmp(transport_name, "tcp")) {
> +#if LIBISCSI_API_VERSION >= (20160603)
> +        transport = TCP_TRANSPORT;
> +    } else if (!strcmp(transport_name, "iser")) {
> +        transport = ISER_TRANSPORT;
> +#else
> +        /* TCP is what older libiscsi versions always use */

Again we should report an error if user set a transport_name
of 'iser' here.

> +#endif
> +    } else {
> +        error_setg(errp, "Unknown transport: %s", transport_name);
>          ret = -EINVAL;
>          goto out;
>      }
>  
>      memset(iscsilun, 0, sizeof(IscsiLun));


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

Re: [Qemu-devel] [RFC PATCH 1/6] iscsi: Split URL into individual options

[Kevin Wolf]

Am 08.12.2016 um 15:10 hat Daniel P. Berrange geschrieben:
> On Thu, Dec 08, 2016 at 02:23:06PM +0100, Kevin Wolf wrote:
> > This introduces a .bdrv_parse_filename handler for iscsi which parses an
> > URL if given and translates it to individual options.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/iscsi.c | 189 ++++++++++++++++++++++++++++++++++++++++++----------------
> >  1 file changed, 136 insertions(+), 53 deletions(-)
> > 
> > diff --git a/block/iscsi.c b/block/iscsi.c
> > index 0960929..7a6664e 100644
> > --- a/block/iscsi.c
> > +++ b/block/iscsi.c
> > @@ -1469,20 +1469,6 @@ static void iscsi_readcapacity_sync(IscsiLun *iscsilun, Error **errp)
> >      }
> >  }
> >  
> > -/* TODO Convert to fine grained options */
> > -static QemuOptsList runtime_opts = {
> > -    .name = "iscsi",
> > -    .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
> > -    .desc = {
> > -        {
> > -            .name = "filename",
> > -            .type = QEMU_OPT_STRING,
> > -            .help = "URL to the iscsi image",
> > -        },
> > -        { /* end of list */ }
> > -    },
> > -};
> > -
> >  static struct scsi_task *iscsi_do_inquiry(struct iscsi_context *iscsi, int lun,
> >                                            int evpd, int pc, void **inq, Error **errp)
> >  {
> > @@ -1604,20 +1590,98 @@ out:
> >   * We support iscsi url's on the form
> >   * iscsi://[<username>%<password>@]<host>[:<port>]/<targetname>/<lun>
> >   */
> > +static void iscsi_parse_filename(const char *filename, QDict *options,
> > +                                 Error **errp)
> > +{
> > +    struct iscsi_url *iscsi_url;
> > +    const char *transport_name;
> > +    char *lun_str;
> > +
> > +    iscsi_url = iscsi_parse_full_url(NULL, filename);
> > +    if (iscsi_url == NULL) {
> > +        error_setg(errp, "Failed to parse URL : %s", filename);
> > +        return;
> > +    }
> > +
> > +#if LIBISCSI_API_VERSION >= (20160603)
> > +    switch (iscsi_url->transport) {
> > +        case TCP_TRANSPORT:     transport_name = "tcp"; break;
> > +        case ISER_TRANSPORT:    transport_name = "iser"; break;
> > +        default:
> > +            error_setg(errp, "Unknown transport type (%d)",
> > +                       iscsi_url->transport);
> > +            return;
> > +    }
> > +#else
> > +    transport_name = "tcp";
> > +#endif
> 
> Here if the URI contained a transport "iser" and we're using
> an old libiscsi, we silently ignore that and use 'tcp'. IMHO
> we should be reporting "Unsupported transport type 'iser'"
> when using the old libiscsi API/

No, old libiscsi would error out in iscsi_parse_full_url(), so you get
the "Failed to parse URL" message from above. Not ideal, but with the
parsing in the library (which I consider a bad idea, but that's how it
works with libiscsi) that's all we can do.

(Okay, not quite true, we could create a context a pass that instead of
NULL, and then we would get access to a slightly better message from
libiscsi. Anyway, we do fail when we should fail, and improving error
messages is out of scope for this series.)

> > +    qdict_set_default_str(options, "transport", transport_name);
> > +    qdict_set_default_str(options, "portal", iscsi_url->portal);
> > +    qdict_set_default_str(options, "target", iscsi_url->target);
> > +
> > +    lun_str = g_strdup_printf("%d", iscsi_url->lun);
> > +    qdict_set_default_str(options, "lun", lun_str);
> > +    g_free(lun_str);
> > +
> > +    if (iscsi_url->user[0] != '\0') {
> > +        qdict_set_default_str(options, "user", iscsi_url->user);
> > +        qdict_set_default_str(options, "password", iscsi_url->passwd);
> 
> If the URI contains "user" but not "password", this stores bogus
> data in the "password" field I believe.

If any field isn't given, it is zeroed (i.e. an empty string); and if
iscsi_url->user is non-empty, iscsi_url->password is non-empty as well.

I basically just used the same condition as the original code, but I
checked it now once more against the libiscsi implementation to be sure.

> > +    }
> > +
> > +    iscsi_destroy_url(iscsi_url);
> > +}
> > +
> > +/* TODO Add -iscsi options */
> > +static QemuOptsList runtime_opts = {
> > +    .name = "iscsi",
> > +    .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
> > +    .desc = {
> > +        {
> > +            .name = "transport",
> > +            .type = QEMU_OPT_STRING,
> > +        },
> > +        {
> > +            .name = "portal",
> > +            .type = QEMU_OPT_STRING,
> > +        },
> > +        {
> > +            .name = "target",
> > +            .type = QEMU_OPT_STRING,
> > +        },
> > +        {
> > +            .name = "user",
> > +            .type = QEMU_OPT_STRING,
> > +        },
> > +        {
> > +            .name = "password",
> > +            .type = QEMU_OPT_STRING,
> > +        },
> > +        {
> > +            .name = "lun",
> > +            .type = QEMU_OPT_NUMBER,
> > +        },
> > +        { /* end of list */ }
> > +    },
> > +};
> > +
> >  static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
> >                        Error **errp)
> >  {
> >      IscsiLun *iscsilun = bs->opaque;
> >      struct iscsi_context *iscsi = NULL;
> > -    struct iscsi_url *iscsi_url = NULL;
> >      struct scsi_task *task = NULL;
> >      struct scsi_inquiry_standard *inq = NULL;
> >      struct scsi_inquiry_supported_pages *inq_vpd;
> >      char *initiator_name = NULL;
> >      QemuOpts *opts;
> >      Error *local_err = NULL;
> > -    const char *filename;
> > -    int i, ret = 0, timeout = 0;
> > +    const char *transport_name, *portal, *target;
> > +    const char *user, *password;
> > +#if LIBISCSI_API_VERSION >= (20160603)
> > +    enum iscsi_transport_type transport;
> > +#endif
> > +    int i, ret = 0, timeout = 0, lun;
> >  
> >      opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
> >      qemu_opts_absorb_qdict(opts, options, &local_err);
> > @@ -1627,18 +1691,41 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
> >          goto out;
> >      }
> >  
> > -    filename = qemu_opt_get(opts, "filename");
> > +    transport_name = qemu_opt_get(opts, "transport");
> > +    portal = qemu_opt_get(opts, "portal");
> > +    target = qemu_opt_get(opts, "target");
> > +    user = qemu_opt_get(opts, "user");
> > +    password = qemu_opt_get(opts, "password");
> > +    lun = qemu_opt_get_number(opts, "lun", 0);
> 
> Do we really want to default to '0' for LUN ?  With Linux tgtd at
> least this is not a valid LUN to use as a volume. It feels like
> we should be raising an error if 'lun' is not explcitly set by
> the user

I wasn't sure about this one. But it's mandatory in the URL, so I guess
you're right that making it mandatory here as well would make sense.

> >  
> > -    iscsi_url = iscsi_parse_full_url(iscsi, filename);
> > -    if (iscsi_url == NULL) {
> > -        error_setg(errp, "Failed to parse URL : %s", filename);
> > +    if (!transport_name || !portal || !target) {
> > +        error_setg(errp, "Need all of transport, portal and target options");
> > +        ret = -EINVAL;
> > +        goto out;
> > +    }
> > +    if (user && !password) {
> > +        error_setg(errp, "If a user name is given, a password is required");
> > +        ret = -EINVAL;
> > +        goto out;
> > +    }
> > +
> > +    if (!strcmp(transport_name, "tcp")) {
> > +#if LIBISCSI_API_VERSION >= (20160603)
> > +        transport = TCP_TRANSPORT;
> > +    } else if (!strcmp(transport_name, "iser")) {
> > +        transport = ISER_TRANSPORT;
> > +#else
> > +        /* TCP is what older libiscsi versions always use */
> 
> Again we should report an error if user set a transport_name
> of 'iser' here.

If you read the code carefully, we do. :-)

A value of "tcp" is ignored and everything else gets the error message
below in the else branch.

> > +#endif
> > +    } else {
> > +        error_setg(errp, "Unknown transport: %s", transport_name);
> >          ret = -EINVAL;
> >          goto out;
> >      }
> >  
> >      memset(iscsilun, 0, sizeof(IscsiLun));

Kevin

[Qemu-devel] [RFC PATCH 2/6] iscsi: Handle -iscsi user/password in bdrv_parse_filename()

[Kevin Wolf]

This splits the logic in the old parse_chap() function into a part that
parses the -iscsi options into the new driver-specific options, and
another part that actually applies those options (called apply_chap()
now).

Note that this means that username and password specified with -iscsi
only take effect when a URL is provided. This is intentional, -iscsi is
a legacy interface only supported for compatibility, new users should
use the proper driver-specific options.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/iscsi.c | 72 +++++++++++++++++++++++++++++++----------------------------
 1 file changed, 38 insertions(+), 34 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 7a6664e..c5106c1 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1235,29 +1235,14 @@ retry:
     return 0;
 }
 
-static void parse_chap(struct iscsi_context *iscsi, const char *target,
+static void apply_chap(struct iscsi_context *iscsi, QemuOpts *opts,
                        Error **errp)
 {
-    QemuOptsList *list;
-    QemuOpts *opts;
     const char *user = NULL;
     const char *password = NULL;
     const char *secretid;
     char *secret = NULL;
 
-    list = qemu_find_opts("iscsi");
-    if (!list) {
-        return;
-    }
-
-    opts = qemu_opts_find(list, target);
-    if (opts == NULL) {
-        opts = QTAILQ_FIRST(&list->head);
-        if (!opts) {
-            return;
-        }
-    }
-
     user = qemu_opt_get(opts, "user");
     if (!user) {
         return;
@@ -1586,6 +1571,35 @@ out:
     }
 }
 
+static void iscsi_parse_iscsi_option(const char *target, QDict *options)
+{
+    QemuOptsList *list;
+    QemuOpts *opts;
+    const char *user;
+
+    list = qemu_find_opts("iscsi");
+    if (!list) {
+        return;
+    }
+
+    opts = qemu_opts_find(list, target);
+    if (opts == NULL) {
+        opts = QTAILQ_FIRST(&list->head);
+        if (!opts) {
+            return;
+        }
+    }
+
+    user = qemu_opt_get(opts, "user");
+    if (user) {
+        qdict_set_default_str(options, "user", user);
+        qdict_set_default_str(options, "password",
+                              qemu_opt_get(opts, "password"));
+        qdict_set_default_str(options, "password-secret",
+                              qemu_opt_get(opts, "password-secret"));
+    }
+}
+
 /*
  * We support iscsi url's on the form
  * iscsi://[<username>%<password>@]<host>[:<port>]/<targetname>/<lun>
@@ -1624,6 +1638,9 @@ static void iscsi_parse_filename(const char *filename, QDict *options,
     qdict_set_default_str(options, "lun", lun_str);
     g_free(lun_str);
 
+    /* User/password from -iscsi take precedence over those from the URL */
+    iscsi_parse_iscsi_option(iscsi_url->target, options);
+
     if (iscsi_url->user[0] != '\0') {
         qdict_set_default_str(options, "user", iscsi_url->user);
         qdict_set_default_str(options, "password", iscsi_url->passwd);
@@ -1658,6 +1675,10 @@ static QemuOptsList runtime_opts = {
             .type = QEMU_OPT_STRING,
         },
         {
+            .name = "password-secret",
+            .type = QEMU_OPT_STRING,
+        },
+        {
             .name = "lun",
             .type = QEMU_OPT_NUMBER,
         },
@@ -1677,7 +1698,6 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     QemuOpts *opts;
     Error *local_err = NULL;
     const char *transport_name, *portal, *target;
-    const char *user, *password;
 #if LIBISCSI_API_VERSION >= (20160603)
     enum iscsi_transport_type transport;
 #endif
@@ -1694,8 +1714,6 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     transport_name = qemu_opt_get(opts, "transport");
     portal = qemu_opt_get(opts, "portal");
     target = qemu_opt_get(opts, "target");
-    user = qemu_opt_get(opts, "user");
-    password = qemu_opt_get(opts, "password");
     lun = qemu_opt_get_number(opts, "lun", 0);
 
     if (!transport_name || !portal || !target) {
@@ -1703,11 +1721,6 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
         ret = -EINVAL;
         goto out;
     }
-    if (user && !password) {
-        error_setg(errp, "If a user name is given, a password is required");
-        ret = -EINVAL;
-        goto out;
-    }
 
     if (!strcmp(transport_name, "tcp")) {
 #if LIBISCSI_API_VERSION >= (20160603)
@@ -1746,17 +1759,8 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
         goto out;
     }
 
-    if (user) {
-        ret = iscsi_set_initiator_username_pwd(iscsi, user, password);
-        if (ret != 0) {
-            error_setg(errp, "Failed to set initiator username and password");
-            ret = -EINVAL;
-            goto out;
-        }
-    }
-
     /* check if we got CHAP username/password via the options */
-    parse_chap(iscsi, target, &local_err);
+    apply_chap(iscsi, opts, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
         ret = -EINVAL;
-- 
1.8.3.1

Re: [Qemu-devel] [RFC PATCH 2/6] iscsi: Handle -iscsi user/password in bdrv_parse_filename()

[Daniel P. Berrange]

On Thu, Dec 08, 2016 at 02:23:07PM +0100, Kevin Wolf wrote:
> This splits the logic in the old parse_chap() function into a part that
> parses the -iscsi options into the new driver-specific options, and
> another part that actually applies those options (called apply_chap()
> now).
> 
> Note that this means that username and password specified with -iscsi
> only take effect when a URL is provided. This is intentional, -iscsi is
> a legacy interface only supported for compatibility, new users should
> use the proper driver-specific options.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/iscsi.c | 72 +++++++++++++++++++++++++++++++----------------------------
>  1 file changed, 38 insertions(+), 34 deletions(-)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 7a6664e..c5106c1 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -1235,29 +1235,14 @@ retry:
>      return 0;
>  }
>  
> -static void parse_chap(struct iscsi_context *iscsi, const char *target,
> +static void apply_chap(struct iscsi_context *iscsi, QemuOpts *opts,
>                         Error **errp)
>  {
> -    QemuOptsList *list;
> -    QemuOpts *opts;
>      const char *user = NULL;
>      const char *password = NULL;
>      const char *secretid;
>      char *secret = NULL;
>  
> -    list = qemu_find_opts("iscsi");
> -    if (!list) {
> -        return;
> -    }
> -
> -    opts = qemu_opts_find(list, target);
> -    if (opts == NULL) {
> -        opts = QTAILQ_FIRST(&list->head);
> -        if (!opts) {
> -            return;
> -        }
> -    }
> -
>      user = qemu_opt_get(opts, "user");
>      if (!user) {
>          return;
> @@ -1586,6 +1571,35 @@ out:
>      }
>  }
>  
> +static void iscsi_parse_iscsi_option(const char *target, QDict *options)
> +{
> +    QemuOptsList *list;
> +    QemuOpts *opts;
> +    const char *user;
> +
> +    list = qemu_find_opts("iscsi");
> +    if (!list) {
> +        return;
> +    }
> +
> +    opts = qemu_opts_find(list, target);
> +    if (opts == NULL) {
> +        opts = QTAILQ_FIRST(&list->head);
> +        if (!opts) {
> +            return;
> +        }
> +    }
> +
> +    user = qemu_opt_get(opts, "user");
> +    if (user) {
> +        qdict_set_default_str(options, "user", user);
> +        qdict_set_default_str(options, "password",
> +                              qemu_opt_get(opts, "password"));
> +        qdict_set_default_str(options, "password-secret",
> +                              qemu_opt_get(opts, "password-secret"));

This core dumps if you set '-iscsi user=foo' but don't set password
or password-secret

#0  0x00007fffda7e2046 in strlen () at /lib64/libc.so.6
#1  0x0000555555b012f9 in qstring_from_str (str=str@entry=0x0) at qobject/qstring.c:66
#2  0x0000555555b02148 in qdict_set_default_str (dst=dst@entry=0x5555566ea7d0, key=key@entry=0x555555b2238b "password", val=0x0) at qobject/qdict.c:490
#3  0x0000555555ab515f in iscsi_parse_iscsi_option (options=0x5555566ea7d0, target=0x5555566f0eb0 "iqn.2004-04.fedora:fedora25:iscsi.kvm") at block/iscsi.c:1545
#4  0x0000555555ab515f in iscsi_parse_filename (filename=<optimized out>, options=0x5555566ea7d0, errp=<optimized out>) at block/iscsi.c:1610


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

[Qemu-devel] [RFC PATCH 3/6] iscsi: Add initiator-name option

[Kevin Wolf]

This was previously only available with -iscsi. Again, after this patch,
the -iscsi option only takes effect if an URL is given. New users are
supposed to use the new driver-specific option.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/iscsi.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index c5106c1..7b909e8 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1311,26 +1311,15 @@ static void parse_header_digest(struct iscsi_context *iscsi, const char *target,
     }
 }
 
-static char *parse_initiator_name(const char *target)
+static char *get_initiator_name(QemuOpts *opts)
 {
-    QemuOptsList *list;
-    QemuOpts *opts;
     const char *name;
     char *iscsi_name;
     UuidInfo *uuid_info;
 
-    list = qemu_find_opts("iscsi");
-    if (list) {
-        opts = qemu_opts_find(list, target);
-        if (!opts) {
-            opts = QTAILQ_FIRST(&list->head);
-        }
-        if (opts) {
-            name = qemu_opt_get(opts, "initiator-name");
-            if (name) {
-                return g_strdup(name);
-            }
-        }
+    name = qemu_opt_get(opts, "initiator-name");
+    if (name) {
+        return g_strdup(name);
     }
 
     uuid_info = qmp_query_uuid(NULL);
@@ -1575,7 +1564,7 @@ static void iscsi_parse_iscsi_option(const char *target, QDict *options)
 {
     QemuOptsList *list;
     QemuOpts *opts;
-    const char *user;
+    const char *user, *initiator_name;
 
     list = qemu_find_opts("iscsi");
     if (!list) {
@@ -1598,6 +1587,11 @@ static void iscsi_parse_iscsi_option(const char *target, QDict *options)
         qdict_set_default_str(options, "password-secret",
                               qemu_opt_get(opts, "password-secret"));
     }
+
+    initiator_name = qemu_opt_get(opts, "initiator-name");
+    if (initiator_name) {
+        qdict_set_default_str(options, "initiator-name", initiator_name);
+    }
 }
 
 /*
@@ -1682,6 +1676,10 @@ static QemuOptsList runtime_opts = {
             .name = "lun",
             .type = QEMU_OPT_NUMBER,
         },
+        {
+            .name = "initiator-name",
+            .type = QEMU_OPT_STRING,
+        },
         { /* end of list */ }
     },
 };
@@ -1738,7 +1736,7 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
 
     memset(iscsilun, 0, sizeof(IscsiLun));
 
-    initiator_name = parse_initiator_name(target);
+    initiator_name = get_initiator_name(opts);
 
     iscsi = iscsi_create_context(initiator_name);
     if (iscsi == NULL) {
-- 
1.8.3.1

Re: [Qemu-devel] [RFC PATCH 3/6] iscsi: Add initiator-name option

[Daniel P. Berrange]

On Thu, Dec 08, 2016 at 02:23:08PM +0100, Kevin Wolf wrote:
> This was previously only available with -iscsi. Again, after this patch,
> the -iscsi option only takes effect if an URL is given. New users are
> supposed to use the new driver-specific option.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/iscsi.c | 32 +++++++++++++++-----------------
>  1 file changed, 15 insertions(+), 17 deletions(-)

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>


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

[Qemu-devel] [RFC PATCH 4/6] iscsi: Add header-digest option

[Kevin Wolf]

This was previously only available with -iscsi. Again, after this patch,
the -iscsi option only takes effect if an URL is given. New users are
supposed to use the new driver-specific option.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/iscsi.c | 38 ++++++++++++++------------------------
 1 file changed, 14 insertions(+), 24 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 7b909e8..f993b19 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1273,32 +1273,15 @@ static void apply_chap(struct iscsi_context *iscsi, QemuOpts *opts,
     g_free(secret);
 }
 
-static void parse_header_digest(struct iscsi_context *iscsi, const char *target,
+static void apply_header_digest(struct iscsi_context *iscsi, QemuOpts *opts,
                                 Error **errp)
 {
-    QemuOptsList *list;
-    QemuOpts *opts;
     const char *digest = NULL;
 
-    list = qemu_find_opts("iscsi");
-    if (!list) {
-        return;
-    }
-
-    opts = qemu_opts_find(list, target);
-    if (opts == NULL) {
-        opts = QTAILQ_FIRST(&list->head);
-        if (!opts) {
-            return;
-        }
-    }
-
     digest = qemu_opt_get(opts, "header-digest");
     if (!digest) {
-        return;
-    }
-
-    if (!strcmp(digest, "CRC32C")) {
+        iscsi_set_header_digest(iscsi, ISCSI_HEADER_DIGEST_NONE_CRC32C);
+    } else if (!strcmp(digest, "CRC32C")) {
         iscsi_set_header_digest(iscsi, ISCSI_HEADER_DIGEST_CRC32C);
     } else if (!strcmp(digest, "NONE")) {
         iscsi_set_header_digest(iscsi, ISCSI_HEADER_DIGEST_NONE);
@@ -1564,7 +1547,7 @@ static void iscsi_parse_iscsi_option(const char *target, QDict *options)
 {
     QemuOptsList *list;
     QemuOpts *opts;
-    const char *user, *initiator_name;
+    const char *user, *initiator_name, *header_digest;
 
     list = qemu_find_opts("iscsi");
     if (!list) {
@@ -1592,6 +1575,11 @@ static void iscsi_parse_iscsi_option(const char *target, QDict *options)
     if (initiator_name) {
         qdict_set_default_str(options, "initiator-name", initiator_name);
     }
+
+    header_digest = qemu_opt_get(opts, "header-digest");
+    if (header_digest) {
+        qdict_set_default_str(options, "header-digest", header_digest);
+    }
 }
 
 /*
@@ -1680,6 +1668,10 @@ static QemuOptsList runtime_opts = {
             .name = "initiator-name",
             .type = QEMU_OPT_STRING,
         },
+        {
+            .name = "header-digest",
+            .type = QEMU_OPT_STRING,
+        },
         { /* end of list */ }
     },
 };
@@ -1771,10 +1763,8 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
         goto out;
     }
 
-    iscsi_set_header_digest(iscsi, ISCSI_HEADER_DIGEST_NONE_CRC32C);
-
     /* check if we got HEADER_DIGEST via the options */
-    parse_header_digest(iscsi, target, &local_err);
+    apply_header_digest(iscsi, opts, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
         ret = -EINVAL;
-- 
1.8.3.1

Re: [Qemu-devel] [RFC PATCH 4/6] iscsi: Add header-digest option

[Daniel P. Berrange]

On Thu, Dec 08, 2016 at 02:23:09PM +0100, Kevin Wolf wrote:
> This was previously only available with -iscsi. Again, after this patch,
> the -iscsi option only takes effect if an URL is given. New users are
> supposed to use the new driver-specific option.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/iscsi.c | 38 ++++++++++++++------------------------
>  1 file changed, 14 insertions(+), 24 deletions(-)

Reviewed-by: Daniel P. berrange <berrange@redhat.com>

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

[Qemu-devel] [RFC PATCH 5/6] iscsi: Add timeout option

[Kevin Wolf]

This was previously only available with -iscsi. Again, after this patch,
the -iscsi option only takes effect if an URL is given. New users are
supposed to use the new driver-specific option.

All -iscsi options have a corresponding driver-specific option for the
iscsi block driver now.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/iscsi.c | 37 +++++++++++--------------------------
 1 file changed, 11 insertions(+), 26 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index f993b19..29191f2 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1317,29 +1317,6 @@ static char *get_initiator_name(QemuOpts *opts)
     return iscsi_name;
 }
 
-static int parse_timeout(const char *target)
-{
-    QemuOptsList *list;
-    QemuOpts *opts;
-    const char *timeout;
-
-    list = qemu_find_opts("iscsi");
-    if (list) {
-        opts = qemu_opts_find(list, target);
-        if (!opts) {
-            opts = QTAILQ_FIRST(&list->head);
-        }
-        if (opts) {
-            timeout = qemu_opt_get(opts, "timeout");
-            if (timeout) {
-                return atoi(timeout);
-            }
-        }
-    }
-
-    return 0;
-}
-
 static void iscsi_nop_timed_event(void *opaque)
 {
     IscsiLun *iscsilun = opaque;
@@ -1547,7 +1524,7 @@ static void iscsi_parse_iscsi_option(const char *target, QDict *options)
 {
     QemuOptsList *list;
     QemuOpts *opts;
-    const char *user, *initiator_name, *header_digest;
+    const char *user, *initiator_name, *header_digest, *timeout;
 
     list = qemu_find_opts("iscsi");
     if (!list) {
@@ -1580,6 +1557,11 @@ static void iscsi_parse_iscsi_option(const char *target, QDict *options)
     if (header_digest) {
         qdict_set_default_str(options, "header-digest", header_digest);
     }
+
+    timeout = qemu_opt_get(opts, "timeout");
+    if (timeout) {
+        qdict_set_default_str(options, "timeout", timeout);
+    }
 }
 
 /*
@@ -1631,7 +1613,6 @@ static void iscsi_parse_filename(const char *filename, QDict *options,
     iscsi_destroy_url(iscsi_url);
 }
 
-/* TODO Add -iscsi options */
 static QemuOptsList runtime_opts = {
     .name = "iscsi",
     .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
@@ -1672,6 +1653,10 @@ static QemuOptsList runtime_opts = {
             .name = "header-digest",
             .type = QEMU_OPT_STRING,
         },
+        {
+            .name = "timeout",
+            .type = QEMU_OPT_NUMBER,
+        },
         { /* end of list */ }
     },
 };
@@ -1772,7 +1757,7 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     /* timeout handling is broken in libiscsi before 1.15.0 */
-    timeout = parse_timeout(target);
+    timeout = qemu_opt_get_number(opts, "timeout", 0);
 #if LIBISCSI_API_VERSION >= 20150621
     iscsi_set_timeout(iscsi, timeout);
 #else
-- 
1.8.3.1

Re: [Qemu-devel] [RFC PATCH 5/6] iscsi: Add timeout option

[Daniel P. Berrange]

On Thu, Dec 08, 2016 at 02:23:10PM +0100, Kevin Wolf wrote:
> This was previously only available with -iscsi. Again, after this patch,
> the -iscsi option only takes effect if an URL is given. New users are
> supposed to use the new driver-specific option.
> 
> All -iscsi options have a corresponding driver-specific option for the
> iscsi block driver now.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/iscsi.c | 37 +++++++++++--------------------------
>  1 file changed, 11 insertions(+), 26 deletions(-)

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>


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

[Qemu-devel] [RFC PATCH 6/6] iscsi: Add blockdev-add support

[Kevin Wolf]

This adds blockdev-add support for iscsi devices.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/iscsi.c        | 14 ++++++----
 qapi/block-core.json | 74 ++++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 78 insertions(+), 10 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 29191f2..6a11cdd 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1281,13 +1281,13 @@ static void apply_header_digest(struct iscsi_context *iscsi, QemuOpts *opts,
     digest = qemu_opt_get(opts, "header-digest");
     if (!digest) {
         iscsi_set_header_digest(iscsi, ISCSI_HEADER_DIGEST_NONE_CRC32C);
-    } else if (!strcmp(digest, "CRC32C")) {
+    } else if (!strcmp(digest, "crc32c")) {
         iscsi_set_header_digest(iscsi, ISCSI_HEADER_DIGEST_CRC32C);
-    } else if (!strcmp(digest, "NONE")) {
+    } else if (!strcmp(digest, "none")) {
         iscsi_set_header_digest(iscsi, ISCSI_HEADER_DIGEST_NONE);
-    } else if (!strcmp(digest, "CRC32C-NONE")) {
+    } else if (!strcmp(digest, "crc32c-none")) {
         iscsi_set_header_digest(iscsi, ISCSI_HEADER_DIGEST_CRC32C_NONE);
-    } else if (!strcmp(digest, "NONE-CRC32C")) {
+    } else if (!strcmp(digest, "none-crc32c")) {
         iscsi_set_header_digest(iscsi, ISCSI_HEADER_DIGEST_NONE_CRC32C);
     } else {
         error_setg(errp, "Invalid header-digest setting : %s", digest);
@@ -1555,7 +1555,11 @@ static void iscsi_parse_iscsi_option(const char *target, QDict *options)
 
     header_digest = qemu_opt_get(opts, "header-digest");
     if (header_digest) {
-        qdict_set_default_str(options, "header-digest", header_digest);
+        /* -iscsi takes upper case values, but QAPI only supports lower case
+         * enum constant names, so we have to convert here. */
+        char *qapi_value = g_ascii_strdown(header_digest, -1);
+        qdict_set_default_str(options, "header-digest", qapi_value);
+        g_free(qapi_value);
     }
 
     timeout = qemu_opt_get(opts, "timeout");
diff --git a/qapi/block-core.json b/qapi/block-core.json
index c29bef7..95a8d23 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1721,10 +1721,10 @@
 { 'enum': 'BlockdevDriver',
   'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop',
             'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom',
-            'host_device', 'http', 'https', 'luks', 'nbd', 'nfs', 'null-aio',
-            'null-co', 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
-            'replication', 'ssh', 'vdi', 'vhdx', 'vmdk', 'vpc',
-            'vvfat' ] }
+            'host_device', 'http', 'https', 'iscsi', 'luks', 'nbd', 'nfs',
+            'null-aio', 'null-co', 'parallels', 'qcow', 'qcow2', 'qed',
+            'quorum', 'raw', 'replication', 'ssh', 'vdi', 'vhdx', 'vmdk',
+            'vpc', 'vvfat' ] }
 
 ##
 # @BlockdevOptionsFile
@@ -2209,6 +2209,70 @@
             '*logfile': 'str' } }
 
 ##
+# @IscsiTransport
+#
+# An enumeration of libiscsi transport types
+#
+# Since: 2.9
+##
+{ 'enum': 'IscsiTransport',
+  'data': [ 'tcp', 'iser' ] }
+
+##
+# @IscsiHeaderDigest
+#
+# An enumeration of header digests supported by libiscsi
+#
+# Since: 2.9
+##
+{ 'enum': 'IscsiHeaderDigest',
+  'prefix': 'QAPI_ISCSI_HEADER_DIGEST',
+  'data': [ 'crc32c', 'none', 'crc32c-none', 'none-crc32c' ] }
+
+##
+# @BlockdevOptionsIscsi
+#
+# @transport        The iscsi transport type
+#
+# @portal           The address of the iscsi portal
+#
+# @target           The target iqn name
+#
+# @lun              #optional LUN to connect to. Defaults to 0.
+#
+# @user             #optional User name to log in with. If omitted, no CHAP
+#                   authentication is performed.
+#
+# @password-secret  #optional The ID of a QCryptoSecret object providing
+#                   the password for the login. This option is required if 
+#                   @user is specified.
+#
+# @initiator-name   #optional The iqn name we want to identify to the target
+#                   as. If this option is not specified, an initiator name is
+#                   generated automatically.
+#
+# @header-digest    #optional The desired header digest. Defaults to
+#                   none-crc32c.
+#
+# @timeout          #optional Timeout in seconds after which a request will
+#                   timeout. 0 means no timeout and is the default.
+#
+# Driver specific block device options for iscsi
+#
+# Since: 2.9
+##
+{ 'struct': 'BlockdevOptionsIscsi',
+  'data': { 'transport': 'IscsiTransport',
+            'portal': 'str',
+            'target': 'str',
+            '*lun': 'int',
+            '*user': 'str',
+            '*password-secret': 'str',
+            '*initiator-name': 'str',
+            '*header-digest': 'IscsiHeaderDigest',
+            '*timeout': 'int' } }
+
+##
 # @ReplicationMode
 #
 # An enumeration of replication modes.
@@ -2394,7 +2458,7 @@
       'host_device':'BlockdevOptionsFile',
       'http':       'BlockdevOptionsCurl',
       'https':      'BlockdevOptionsCurl',
-# TODO iscsi: Wait for structured options
+      'iscsi':      'BlockdevOptionsIscsi',
       'luks':       'BlockdevOptionsLUKS',
       'nbd':        'BlockdevOptionsNbd',
       'nfs':        'BlockdevOptionsNfs',
-- 
1.8.3.1

Re: [Qemu-devel] [RFC PATCH 6/6] iscsi: Add blockdev-add support

[Daniel P. Berrange]

On Thu, Dec 08, 2016 at 02:23:11PM +0100, Kevin Wolf wrote:
> This adds blockdev-add support for iscsi devices.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/iscsi.c        | 14 ++++++----
>  qapi/block-core.json | 74 ++++++++++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 78 insertions(+), 10 deletions(-)

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>

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

Re: [Qemu-devel] [RFC PATCH 6/6] iscsi: Add blockdev-add support

[Eric Blake]

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

On 12/08/2016 07:23 AM, Kevin Wolf wrote:
> This adds blockdev-add support for iscsi devices.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/iscsi.c        | 14 ++++++----
>  qapi/block-core.json | 74 ++++++++++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 78 insertions(+), 10 deletions(-)
> 

> +++ b/qapi/block-core.json
> @@ -1721,10 +1721,10 @@
>  { 'enum': 'BlockdevDriver',
>    'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop',
>              'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom',
> -            'host_device', 'http', 'https', 'luks', 'nbd', 'nfs', 'null-aio',
> -            'null-co', 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
> -            'replication', 'ssh', 'vdi', 'vhdx', 'vmdk', 'vpc',
> -            'vvfat' ] }
> +            'host_device', 'http', 'https', 'iscsi', 'luks', 'nbd', 'nfs',
> +            'null-aio', 'null-co', 'parallels', 'qcow', 'qcow2', 'qed',
> +            'quorum', 'raw', 'replication', 'ssh', 'vdi', 'vhdx', 'vmdk',
> +            'vpc', 'vvfat' ] }

Missing documentation that calls out 'since 2.9'

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [RFC PATCH 0/6] iscsi: Add blockdev-add support

[Daniel P. Berrange]

On Thu, Dec 08, 2016 at 02:23:05PM +0100, Kevin Wolf wrote:
> This adds blockdev-add support to the iscsi block driver.
> 
> Note that this is only compile tested at this point. Jeff is going to
> take over from here and bring the series to a mergable state.
> 
> Kevin Wolf (6):
>   iscsi: Split URL into individual options
>   iscsi: Handle -iscsi user/password in bdrv_parse_filename()
>   iscsi: Add initiator-name option
>   iscsi: Add header-digest option
>   iscsi: Add timeout option
>   iscsi: Add blockdev-add support
> 
>  block/iscsi.c        | 342 ++++++++++++++++++++++++++++++---------------------
>  qapi/block-core.json |  74 ++++++++++-
>  2 files changed, 272 insertions(+), 144 deletions(-)

This series works as well as my series does, once you apply this fix
for the crash bug I mention:

diff --git a/block/iscsi.c b/block/iscsi.c
index 6a11cdd..320e56a 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1524,7 +1524,7 @@ static void iscsi_parse_iscsi_option(const char *target, QDict *options)
 {
     QemuOptsList *list;
     QemuOpts *opts;
-    const char *user, *initiator_name, *header_digest, *timeout;
+    const char *user, *initiator_name, *header_digest, *timeout, *password, *password_secret;
 
     list = qemu_find_opts("iscsi");
     if (!list) {
@@ -1542,10 +1542,14 @@ static void iscsi_parse_iscsi_option(const char *target, QDict *options)
     user = qemu_opt_get(opts, "user");
     if (user) {
         qdict_set_default_str(options, "user", user);
-        qdict_set_default_str(options, "password",
-                              qemu_opt_get(opts, "password"));
-        qdict_set_default_str(options, "password-secret",
-                              qemu_opt_get(opts, "password-secret"));
+    }
+    password = qemu_opt_get(opts, "password");
+    if (password) {
+        qdict_set_default_str(options, "password", password);
+    }
+    password_secret = qemu_opt_get(opts, "password-secret");
+    if (password_secret) {
+        qdict_set_default_str(options, "password-secret", password_secret);
     }
 
     initiator_name = qemu_opt_get(opts, "initiator-name");


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

Re: [Qemu-devel] [RFC PATCH 0/6] iscsi: Add blockdev-add support

[Kevin Wolf]

Am 08.12.2016 um 14:55 hat Daniel P. Berrange geschrieben:
> On Thu, Dec 08, 2016 at 02:23:05PM +0100, Kevin Wolf wrote:
> > This adds blockdev-add support to the iscsi block driver.
> > 
> > Note that this is only compile tested at this point. Jeff is going to
> > take over from here and bring the series to a mergable state.
> > 
> > Kevin Wolf (6):
> >   iscsi: Split URL into individual options
> >   iscsi: Handle -iscsi user/password in bdrv_parse_filename()
> >   iscsi: Add initiator-name option
> >   iscsi: Add header-digest option
> >   iscsi: Add timeout option
> >   iscsi: Add blockdev-add support
> > 
> >  block/iscsi.c        | 342 ++++++++++++++++++++++++++++++---------------------
> >  qapi/block-core.json |  74 ++++++++++-
> >  2 files changed, 272 insertions(+), 144 deletions(-)
> 
> This series works as well as my series does, once you apply this fix
> for the crash bug I mention:
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 6a11cdd..320e56a 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -1524,7 +1524,7 @@ static void iscsi_parse_iscsi_option(const char *target, QDict *options)
>  {
>      QemuOptsList *list;
>      QemuOpts *opts;
> -    const char *user, *initiator_name, *header_digest, *timeout;
> +    const char *user, *initiator_name, *header_digest, *timeout, *password, *password_secret;
>  
>      list = qemu_find_opts("iscsi");
>      if (!list) {
> @@ -1542,10 +1542,14 @@ static void iscsi_parse_iscsi_option(const char *target, QDict *options)
>      user = qemu_opt_get(opts, "user");
>      if (user) {
>          qdict_set_default_str(options, "user", user);
> -        qdict_set_default_str(options, "password",
> -                              qemu_opt_get(opts, "password"));
> -        qdict_set_default_str(options, "password-secret",
> -                              qemu_opt_get(opts, "password-secret"));
> +    }
> +    password = qemu_opt_get(opts, "password");
> +    if (password) {
> +        qdict_set_default_str(options, "password", password);
> +    }
> +    password_secret = qemu_opt_get(opts, "password-secret");
> +    if (password_secret) {
> +        qdict_set_default_str(options, "password-secret", password_secret);
>      }

Looks good to me. Though I would keep these inside the 'if (user)'
block.

The driver silently ignores password/password-secret if user isn't given
(this is how it worked even before this patch). The interesting case
where it makes a difference is if you give one option directly to the
block driver and the other one with -iscsi. I don't think we want to
allow that, both options should come from the same place.

Kevin

Re: [Qemu-devel] [RFC PATCH 0/6] iscsi: Add blockdev-add support

[Daniel P. Berrange]

On Thu, Dec 08, 2016 at 02:23:05PM +0100, Kevin Wolf wrote:
> This adds blockdev-add support to the iscsi block driver.
> 
> Note that this is only compile tested at this point. Jeff is going to
> take over from here and bring the series to a mergable state.

I've not seen any updates to this and soft-freeze is fast approaching.

I really badly want to see this fixed in QEMU 2.9 so libvirt can use
it to fix iSCSI authentication support.

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

Re: [Qemu-devel] [RFC PATCH 0/6] iscsi: Add blockdev-add support

[Jeff Cody]

On Tue, Jan 24, 2017 at 11:35:12AM +0000, Daniel P. Berrange wrote:
> On Thu, Dec 08, 2016 at 02:23:05PM +0100, Kevin Wolf wrote:
> > This adds blockdev-add support to the iscsi block driver.
> > 
> > Note that this is only compile tested at this point. Jeff is going to
> > take over from here and bring the series to a mergable state.
> 
> I've not seen any updates to this and soft-freeze is fast approaching.
> 
> I really badly want to see this fixed in QEMU 2.9 so libvirt can use
> it to fix iSCSI authentication support.
> 
> Regards,
> Daniel

Hi Daniel,

Yep, I am working on getting the patches out.  It should be out soon.

Jeff