Remove need for -iscsi argument

PATCH In review Daniel P. Berrange 2 participants 5 messages Dec 8, 2016 Block layer

Share & tools

Daniel P. Berrange

author

2016-12-08 12:41 UTC #0 lore raw

cover Remove need for -iscsi argument

This small series removes the need for using the -iscsi argument by directly supporting all the -iscsi options against the block device config eg instead of

$QEMU \
-object secret,id=sec0,data=123456 \
-drive driver=iscsi,file=iscsi://catbus/iqn.fedora:kvm/2 \
-iscsi initiator-name=fish,user=test,password-secret=sec0,timeout=234

you can now do it all against -drive

$QEMU \
-object secret,id=sec0,data=123456 \
-drive driver=iscsi,file=iscsi://catbus/iqn.fedora:kvm/2,\
initiator-name=fish,user=test,password-secret=sec0,timeout=234

This fixes the problem that you cannot associate -drive instances with -iscsi args reliably due to IQNs containing chracters that QEMU forbids in QemuOpts ID strings.

Pino had previously sent a patch to do this for initiator-name back in the 2.6 dev cycle

https://lists.nongnu.org/archive/html/qemu-block/2016-04/msg00246.html

This series supports all the -iscsi option names, except for 'password' which is intentionally left out as it is insecure and replaced by 'password-secret'.

Daniel P. Berrange (2):
iscsi: reduce code duplication parsing -iscsi opts
iscsi: support most -iscsi opts against block dev opts

 block/iscsi.c | 145 ++++++++++++++++++++++++++++++----------------------------
 block/iscsi.h |  79 ++++++++++++++++++++++++++++++++
 vl.c          |  36 ++++-----------
 3 files changed, 163 insertions(+), 97 deletions(-)
 create mode 100644 block/iscsi.h

Signature · 1 line

2.9.3

Daniel P. Berrange

author

2016-12-08 12:41 UTC #1 lore raw

PATCH 1/2 iscsi: reduce code duplication parsing -iscsi opts

The iscsi block driver has to pull some of its options out of a separate -iscsi. It has a separate helper method for each set of options it needs to extract, and in doing so, it runs qemu_opts_find multiple times. Switch to running qemu_opts_find once and passing the QemuOpts object into the helpers instead.

Signed-off-by Daniel P. Berrange <[email protected]>

---
 block/iscsi.c | 112 +++++++++++++++++++++++++---------------------------------
 1 file changed, 48 insertions(+), 64 deletions(-)

block/iscsi.c +48 ~~-64~~

index 0960929..6c5bb89 100644
@@ -1235,36 +1235,25 @@ retry:
     return 0;
 }
 
-static void parse_chap(struct iscsi_context *iscsi, const char *target,
+static void parse_chap(QemuOpts *iscsiopts, struct iscsi_context *iscsi,
                        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) {
+    if (!iscsiopts) {
         return;
     }
 
-    opts = qemu_opts_find(list, target);
-    if (opts == NULL) {
-        opts = QTAILQ_FIRST(&list->head);
-        if (!opts) {
-            return;
-        }
-    }
-
-    user = qemu_opt_get(opts, "user");
+    user = qemu_opt_get(iscsiopts, "user");
     if (!user) {
         return;
     }
 
-    secretid = qemu_opt_get(opts, "password-secret");
-    password = qemu_opt_get(opts, "password");
+    secretid = qemu_opt_get(iscsiopts, "password-secret");
+    password = qemu_opt_get(iscsiopts, "password");
     if (secretid && password) {
         error_setg(errp, "'password' and 'password-secret' properties are "
                    "mutually exclusive");

@@ -1288,27 +1277,16 @@ static void parse_chap(struct iscsi_context *iscsi, const char *target,
     g_free(secret);
 }
 
-static void parse_header_digest(struct iscsi_context *iscsi, const char *target,
-                                Error **errp)
+static void parse_header_digest(QemuOpts *iscsiopts,
+                                struct iscsi_context *iscsi, Error **errp)
 {
-    QemuOptsList *list;
-    QemuOpts *opts;
     const char *digest = NULL;
 
-    list = qemu_find_opts("iscsi");
-    if (!list) {
+    if (!iscsiopts) {
         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");
+    digest = qemu_opt_get(iscsiopts, "header-digest");
     if (!digest) {
         return;
     }

@@ -1326,25 +1304,16 @@ 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(QemuOpts *iscsiopts)
 {
-    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);
-            }
+    if (iscsiopts) {
+        name = qemu_opt_get(iscsiopts, "initiator-name");
+        if (name) {
+            return g_strdup(name);
         }
     }

@@ -1360,23 +1329,14 @@ static char *parse_initiator_name(const char *target)
     return iscsi_name;
 }
 
-static int parse_timeout(const char *target)
+static int parse_timeout(QemuOpts *iscsiopts)
 {
-    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);
-            }
+    if (iscsiopts) {
+        timeout = qemu_opt_get(iscsiopts, "timeout");
+        if (timeout) {
+            return atoi(timeout);
         }
     }

@@ -1600,6 +1560,28 @@ out:
     }
 }
 
+static QemuOpts *find_iscsi_opts(const char *target)
+{
+    QemuOptsList *list;
+    QemuOpts *opts;
+
+    list = qemu_find_opts("iscsi");
+    if (!list) {
+        return NULL;
+    }
+
+    opts = qemu_opts_find(list, target);
+    if (opts == NULL) {
+        opts = QTAILQ_FIRST(&list->head);
+        if (!opts) {
+            return NULL;
+        }
+    }
+
+    return opts;
+}
+
+
 /*
  * We support iscsi url's on the form
  * iscsi://[<username>%<password>@]<host>[:<port>]/<targetname>/<lun>

@@ -1614,7 +1596,7 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     struct scsi_inquiry_standard *inq = NULL;
     struct scsi_inquiry_supported_pages *inq_vpd;
     char *initiator_name = NULL;
-    QemuOpts *opts;
+    QemuOpts *opts, *iscsiopts;
     Error *local_err = NULL;
     const char *filename;
     int i, ret = 0, timeout = 0;

@@ -1636,9 +1618,11 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
         goto out;
     }
 
+    iscsiopts = find_iscsi_opts(iscsi_url->target);
+
     memset(iscsilun, 0, sizeof(IscsiLun));
 
-    initiator_name = parse_initiator_name(iscsi_url->target);
+    initiator_name = parse_initiator_name(iscsiopts);
 
     iscsi = iscsi_create_context(initiator_name);
     if (iscsi == NULL) {

@@ -1670,7 +1654,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(iscsiopts, iscsi, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
         ret = -EINVAL;

@@ -1686,7 +1670,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(iscsiopts, iscsi, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
         ret = -EINVAL;

@@ -1694,7 +1678,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(iscsiopts);
 #if LIBISCSI_API_VERSION >= 20150621
     iscsi_set_timeout(iscsi, timeout);
 #else

Signature · 1 line

2.9.3

Daniel P. Berrange

author

2016-12-08 12:41 UTC #2 lore raw

PATCH 2/2 iscsi: support most -iscsi opts against block dev opts

In reply to Daniel P. Berrange

Currently a number of iscsi related options must be specified against the separate -iscsi argument, instead of as part of the block device configuration. This extends the iscsi bdev runtime options to support all the options available via -iscsi, except for the deprecated "password" option. This makes the -iscsi argument obsolete, though it is kept for sake of backwards compatibility.

If the same property is set against -iscsi and against -drive, then the latter will always take priority.

Signed-off-by Daniel P. Berrange <[email protected]>

---
 block/iscsi.c | 75 ++++++++++++++++++++++++++++++++++++--------------------
 block/iscsi.h | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 vl.c          | 36 +++++++--------------------
 3 files changed, 136 insertions(+), 54 deletions(-)
 create mode 100644 block/iscsi.h

block/iscsi.c +48 ~~-27~~

index 6c5bb89..939150f 100644
@@ -40,6 +40,7 @@
 #include "qmp-commands.h"
 #include "qapi/qmp/qstring.h"
 #include "crypto/secret.h"
+#include "block/iscsi.h"
 
 #include <iscsi/iscsi.h>
 #include <iscsi/scsi-lowlevel.h>

@@ -1235,25 +1236,34 @@ retry:
     return 0;
 }
 
-static void parse_chap(QemuOpts *iscsiopts, struct iscsi_context *iscsi,
-                       Error **errp)
+static void parse_chap(QemuOpts *iscsiopts, QemuOpts *bdevopts,
+                       struct iscsi_context *iscsi, Error **errp)
 {
     const char *user = NULL;
     const char *password = NULL;
     const char *secretid;
     char *secret = NULL;
 
-    if (!iscsiopts) {
-        return;
+    user = qemu_opt_get(bdevopts, "user");
+    if (!user && iscsiopts) {
+        user = qemu_opt_get(iscsiopts, "user");
     }
-
-    user = qemu_opt_get(iscsiopts, "user");
     if (!user) {
         return;
     }
 
-    secretid = qemu_opt_get(iscsiopts, "password-secret");
-    password = qemu_opt_get(iscsiopts, "password");
+    secretid = qemu_opt_get(bdevopts, "password-secret");
+    if (!secretid && iscsiopts) {
+        secretid = qemu_opt_get(iscsiopts, "password-secret");
+    }
+    /* Intentionally don't look for 'password' in bdev opts,
+     * since this is an insecure broken feature of -iscsi.
+     * we only want to support password-secret with -drive
+     */
+    if (iscsiopts) {
+        password = qemu_opt_get(iscsiopts, "password");
+    }
+
     if (secretid && password) {
         error_setg(errp, "'password' and 'password-secret' properties are "
                    "mutually exclusive");

@@ -1277,16 +1287,15 @@ static void parse_chap(QemuOpts *iscsiopts, struct iscsi_context *iscsi,
     g_free(secret);
 }
 
-static void parse_header_digest(QemuOpts *iscsiopts,
+static void parse_header_digest(QemuOpts *iscsiopts, QemuOpts *bdevopts,
                                 struct iscsi_context *iscsi, Error **errp)
 {
     const char *digest = NULL;
 
-    if (!iscsiopts) {
-        return;
+    digest = qemu_opt_get(bdevopts, "header-digest");
+    if (!digest && iscsiopts) {
+        digest = qemu_opt_get(iscsiopts, "header-digest");
     }
-
-    digest = qemu_opt_get(iscsiopts, "header-digest");
     if (!digest) {
         return;
     }

@@ -1304,17 +1313,18 @@ static void parse_header_digest(QemuOpts *iscsiopts,
     }
 }
 
-static char *parse_initiator_name(QemuOpts *iscsiopts)
+static char *parse_initiator_name(QemuOpts *iscsiopts, QemuOpts *bdevopts)
 {
     const char *name;
     char *iscsi_name;
     UuidInfo *uuid_info;
 
-    if (iscsiopts) {
+    name = qemu_opt_get(bdevopts, "initiator-name");
+    if (!name && iscsiopts) {
         name = qemu_opt_get(iscsiopts, "initiator-name");
-        if (name) {
-            return g_strdup(name);
-        }
+    }
+    if (name) {
+        return g_strdup(name);
     }
 
     uuid_info = qmp_query_uuid(NULL);

@@ -1329,15 +1339,16 @@ static char *parse_initiator_name(QemuOpts *iscsiopts)
     return iscsi_name;
 }
 
-static int parse_timeout(QemuOpts *iscsiopts)
+static int parse_timeout(QemuOpts *iscsiopts, QemuOpts *bdevopts)
 {
     const char *timeout;
 
-    if (iscsiopts) {
+    timeout = qemu_opt_get(bdevopts, "timeout");
+    if (!timeout && iscsiopts) {
         timeout = qemu_opt_get(iscsiopts, "timeout");
-        if (timeout) {
-            return atoi(timeout);
-        }
+    }
+    if (timeout) {
+        return atoi(timeout);
     }
 
     return 0;

@@ -1439,6 +1450,16 @@ static QemuOptsList runtime_opts = {
             .type = QEMU_OPT_STRING,
             .help = "URL to the iscsi image",
         },
+        ISCSI_OPT_USER,
+        /* Intentionally don't register 'password' in bdev opts,
+         * since this is an insecure broken feature of -iscsi.
+         * we only want to support password-secret with -drive
+         *
+         *ISCSI_OPT_PASSWORD,*/
+        ISCSI_OPT_PASSWORD_SECRET,
+        ISCSI_OPT_HEADER_DIGEST,
+        ISCSI_OPT_INITIATOR_NAME,
+        ISCSI_OPT_TIMEOUT,
         { /* end of list */ }
     },
 };

@@ -1622,7 +1643,7 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
 
     memset(iscsilun, 0, sizeof(IscsiLun));
 
-    initiator_name = parse_initiator_name(iscsiopts);
+    initiator_name = parse_initiator_name(iscsiopts, opts);
 
     iscsi = iscsi_create_context(initiator_name);
     if (iscsi == NULL) {

@@ -1654,7 +1675,7 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     /* check if we got CHAP username/password via the options */
-    parse_chap(iscsiopts, iscsi, &local_err);
+    parse_chap(iscsiopts, opts, iscsi, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
         ret = -EINVAL;

@@ -1670,7 +1691,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(iscsiopts, iscsi, &local_err);
+    parse_header_digest(iscsiopts, opts, iscsi, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
         ret = -EINVAL;

@@ -1678,7 +1699,7 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     /* timeout handling is broken in libiscsi before 1.15.0 */
-    timeout = parse_timeout(iscsiopts);
+    timeout = parse_timeout(iscsiopts, opts);
 #if LIBISCSI_API_VERSION >= 20150621
     iscsi_set_timeout(iscsi, timeout);
 #else

block/iscsi.h new file +79 -0

79 lines not on this page · raw message

vl.c +9 ~~-27~~

index d77dd86..7234d9c 100644
@@ -26,6 +26,9 @@
 #include "qemu/cutils.h"
 #include "qemu/help_option.h"
 #include "qemu/uuid.h"
+#ifdef CONFIG_LIBISCSI
+#include "block/iscsi.h"
+#endif
 
 #ifdef CONFIG_SECCOMP
 #include "sysemu/seccomp.h"

@@ -514,33 +517,12 @@ static QemuOptsList qemu_iscsi_opts = {
     .name = "iscsi",
     .head = QTAILQ_HEAD_INITIALIZER(qemu_iscsi_opts.head),
     .desc = {
-        {
-            .name = "user",
-            .type = QEMU_OPT_STRING,
-            .help = "username for CHAP authentication to target",
-        },{
-            .name = "password",
-            .type = QEMU_OPT_STRING,
-            .help = "password for CHAP authentication to target",
-        },{
-            .name = "password-secret",
-            .type = QEMU_OPT_STRING,
-            .help = "ID of the secret providing password for CHAP "
-                    "authentication to target",
-        },{
-            .name = "header-digest",
-            .type = QEMU_OPT_STRING,
-            .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)",
-        },
+        ISCSI_OPT_USER,
+        ISCSI_OPT_PASSWORD,
+        ISCSI_OPT_PASSWORD_SECRET,
+        ISCSI_OPT_HEADER_DIGEST,
+        ISCSI_OPT_INITIATOR_NAME,
+        ISCSI_OPT_TIMEOUT,
         { /* end of list */ }
     },
 };

Signature · 1 line

2.9.3

Kevin Wolf

2016-12-08 13:12 UTC #3 lore raw

In reply to Daniel P. Berrange

Am 08.12.2016 um 13:41 hat Daniel P. Berrange geschrieben:

Quoted 20 lines

This small series removes the need for using the -iscsi argument by directly supporting all the -iscsi options against the block device config eg instead of
$QEMU \
-object secret,id=sec0,data=123456 \
-drive driver=iscsi,file=iscsi://catbus/iqn.fedora:kvm/2 \
-iscsi initiator-name=fish,user=test,password-secret=sec0,timeout=234
you can now do it all against -drive
$QEMU \
-object secret,id=sec0,data=123456 \
-drive driver=iscsi,file=iscsi://catbus/iqn.fedora:kvm/2,\
initiator-name=fish,user=test,password-secret=sec0,timeout=234
This fixes the problem that you cannot associate -drive instances with -iscsi args reliably due to IQNs containing chracters that QEMU forbids in QemuOpts ID strings.

Quoted 8 lines

Pino had previously sent a patch to do this for initiator-name back in the 2.6 dev cycle
https://lists.nongnu.org/archive/html/qemu-block/2016-04/msg00246.html
This series supports all the -iscsi option names, except for 'password' which is intentionally left out as it is insecure and replaced by 'password-secret'.

I actually do the same as part of my iscsi for blockdev-add series, which I guess I better should have sent as an RFC even if it's yet untested to save you the work...

As I do more on top of just converting -iscsi, I would prefer using my patches even if you posted yours first.

Kevin

Daniel P. Berrange

author

2016-12-08 13:16 UTC #4 lore raw

Quoted 30 lines · Kevin Wolf

Am 08.12.2016 um 13:41 hat Daniel P. Berrange geschrieben:
Quoted 20 lines
This small series removes the need for using the -iscsi argument by directly supporting all the -iscsi options against the block device config eg instead of
$QEMU \
-object secret,id=sec0,data=123456 \
-drive driver=iscsi,file=iscsi://catbus/iqn.fedora:kvm/2 \
-iscsi initiator-name=fish,user=test,password-secret=sec0,timeout=234
you can now do it all against -drive
$QEMU \
-object secret,id=sec0,data=123456 \
-drive driver=iscsi,file=iscsi://catbus/iqn.fedora:kvm/2,\
initiator-name=fish,user=test,password-secret=sec0,timeout=234
This fixes the problem that you cannot associate -drive instances with -iscsi args reliably due to IQNs containing chracters that QEMU forbids in QemuOpts ID strings.
Quoted 8 lines
Pino had previously sent a patch to do this for initiator-name back in the 2.6 dev cycle
https://lists.nongnu.org/archive/html/qemu-block/2016-04/msg00246.html
This series supports all the -iscsi option names, except for 'password' which is intentionally left out as it is insecure and replaced by 'password-secret'.

Quoted 6 lines

I actually do the same as part of my iscsi for blockdev-add series, which I guess I better should have sent as an RFC even if it's yet untested to save you the work...
As I do more on top of just converting -iscsi, I would prefer using my patches even if you posted yours first.

I don't mind, as long as this gets done for 2.9 and doesn't slip for yet another release.

Regards,
Daniel

Signature · 3 lines

|: 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/ :|