[Qemu-devel] [PATCH 0/3] iscsi: add -drive ..., iscsi=<iscsi-id> parameter

[Qemu-devel] [PATCH 0/3] iscsi: add -drive ..., iscsi=<iscsi-id> parameter

[Stefan Hajnoczi]

Add a parameter to associate an iSCSI block driver instance with an -iscsi
object.  This is more powerful than relying on the implicit target IQN naming
convention since that only allows one -iscsi object per target.

Stefan Hajnoczi (3):
  iscsi: extract find_iscsi_opts() helper function
  iscsi: simplify -iscsi QemuOpts fetching
  iscsi: add iscsi=<iscsi-id> option

 block/iscsi.c   | 113 +++++++++++++++++++++++---------------------------------
 qemu-options.hx |   8 +++-
 2 files changed, 52 insertions(+), 69 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/3] iscsi: extract find_iscsi_opts() helper function

[Stefan Hajnoczi]

Extract a function that looks up the -iscsi object to avoid duplicating
this code.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/iscsi.c | 82 ++++++++++++++++++++++++++---------------------------------
 1 file changed, 36 insertions(+), 46 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 95ce9e1..a214e3f 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1198,29 +1198,42 @@ retry:
     return 0;
 }
 
+/* Look up the -iscsi command-line option */
+static QemuOpts *find_iscsi_opts(const char *id)
+{
+    QemuOptsList *list;
+    QemuOpts *opts;
+
+    list = qemu_find_opts("iscsi");
+    if (!list) {
+        return NULL;
+    }
+
+    opts = qemu_opts_find(list, id);
+    if (opts == NULL) {
+        opts = QTAILQ_FIRST(&list->head);
+        if (!opts) {
+            return NULL;
+        }
+    }
+
+    return opts;
+}
+
 static void parse_chap(struct iscsi_context *iscsi, const char *target,
                        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) {
+    opts = find_iscsi_opts(target);
+    if (!opts) {
         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;
@@ -1254,23 +1267,14 @@ static void parse_chap(struct iscsi_context *iscsi, const char *target,
 static void parse_header_digest(struct iscsi_context *iscsi, const char *target,
                                 Error **errp)
 {
-    QemuOptsList *list;
     QemuOpts *opts;
     const char *digest = NULL;
 
-    list = qemu_find_opts("iscsi");
-    if (!list) {
+    opts = find_iscsi_opts(target);
+    if (!opts) {
         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;
@@ -1291,23 +1295,16 @@ static void parse_header_digest(struct iscsi_context *iscsi, const char *target,
 
 static char *parse_initiator_name(const char *target)
 {
-    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);
-            }
+    opts = find_iscsi_opts(target);
+    if (opts) {
+        name = qemu_opt_get(opts, "initiator-name");
+        if (name) {
+            return g_strdup(name);
         }
     }
 
@@ -1325,21 +1322,14 @@ static char *parse_initiator_name(const char *target)
 
 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);
-            }
+    opts = find_iscsi_opts(target);
+    if (opts) {
+        timeout = qemu_opt_get(opts, "timeout");
+        if (timeout) {
+            return atoi(timeout);
         }
     }
 
-- 
2.7.4

[Qemu-devel] [PATCH 2/3] iscsi: simplify -iscsi QemuOpts fetching

[Stefan Hajnoczi]

Instead of looking up the -iscsi option multiple times, look it up once
and pass the QemuOpts pointer around.  This results in shorter, simpler
code.

Note that this patch also take advantage of the fact that
qemu_opt_get(NULL, "foo") returns NULL.  Therefore it's not necessary to
check that opts != NULL before calling qemu_opt_get().

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/iscsi.c | 52 +++++++++++++++++-----------------------------------
 1 file changed, 17 insertions(+), 35 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index a214e3f..4eb9a36 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1220,20 +1220,14 @@ static QemuOpts *find_iscsi_opts(const char *id)
     return opts;
 }
 
-static void parse_chap(struct iscsi_context *iscsi, const char *target,
+static void parse_chap(struct iscsi_context *iscsi, QemuOpts *opts,
                        Error **errp)
 {
-    QemuOpts *opts;
     const char *user = NULL;
     const char *password = NULL;
     const char *secretid;
     char *secret = NULL;
 
-    opts = find_iscsi_opts(target);
-    if (!opts) {
-        return;
-    }
-
     user = qemu_opt_get(opts, "user");
     if (!user) {
         return;
@@ -1264,17 +1258,11 @@ 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,
+static void parse_header_digest(struct iscsi_context *iscsi, QemuOpts *opts,
                                 Error **errp)
 {
-    QemuOpts *opts;
     const char *digest = NULL;
 
-    opts = find_iscsi_opts(target);
-    if (!opts) {
-        return;
-    }
-
     digest = qemu_opt_get(opts, "header-digest");
     if (!digest) {
         return;
@@ -1293,19 +1281,15 @@ 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 *opts)
 {
-    QemuOpts *opts;
     const char *name;
     char *iscsi_name;
     UuidInfo *uuid_info;
 
-    opts = find_iscsi_opts(target);
-    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);
@@ -1320,19 +1304,14 @@ static char *parse_initiator_name(const char *target)
     return iscsi_name;
 }
 
-static int parse_timeout(const char *target)
+static int parse_timeout(QemuOpts *opts)
 {
-    QemuOpts *opts;
     const char *timeout;
 
-    opts = find_iscsi_opts(target);
-    if (opts) {
-        timeout = qemu_opt_get(opts, "timeout");
-        if (timeout) {
-            return atoi(timeout);
-        }
+    timeout = qemu_opt_get(opts, "timeout");
+    if (timeout) {
+        return atoi(timeout);
     }
-
     return 0;
 }
 
@@ -1568,6 +1547,7 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     struct scsi_inquiry_supported_pages *inq_vpd;
     char *initiator_name = NULL;
     QemuOpts *opts;
+    QemuOpts *iscsi_opts;
     Error *local_err = NULL;
     const char *filename;
     int i, ret = 0, timeout = 0;
@@ -1591,7 +1571,9 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
 
     memset(iscsilun, 0, sizeof(IscsiLun));
 
-    initiator_name = parse_initiator_name(iscsi_url->target);
+    iscsi_opts = find_iscsi_opts(iscsi_url->target);
+
+    initiator_name = parse_initiator_name(iscsi_opts);
 
     iscsi = iscsi_create_context(initiator_name);
     if (iscsi == NULL) {
@@ -1617,7 +1599,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, iscsi_opts, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
         ret = -EINVAL;
@@ -1633,7 +1615,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, iscsi_opts, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
         ret = -EINVAL;
@@ -1641,7 +1623,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(iscsi_opts);
 #if defined(LIBISCSI_API_VERSION) && LIBISCSI_API_VERSION >= 20150621
     iscsi_set_timeout(iscsi, timeout);
 #else
-- 
2.7.4

[Qemu-devel] [PATCH 3/3] iscsi: add iscsi=<iscsi-id> option

[Stefan Hajnoczi]

An iSCSI block driver instance is implicitly matched with an -iscsi
object that has id=<target-iqn>.  If no -iscsi object with that id
exists then the first -iscsi object is used.

This patch adds a -drive ...,iscsi=<iscsi-id> option so the relationship
between the block driver instance and an -iscsi object can be specified
explicitly.  When the iscsi=<iscsi-id> parameter is used there is no
fallback to the first -iscsi option (which is weird legacy behavior).

This makes it possible to connect to multiple LUNs on the same target
with different credentials whereas previously the same -iscsi object
would be associated with all block driver instances for that iSCSI
target.

Example:

  $ qemu-system-x86_64 -iscsi id=test,user=foo,password=x,\
          initiator-name=iqn.2001-04.com.example:my-initiator \
          -drive driver=iscsi,filename=iscsi://127.0.0.1/iqn.2003-01.org.linux-iscsi.test.x8664:sn.d964cc832811/0,iscsi=test

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/iscsi.c   | 21 ++++++++++++++-------
 qemu-options.hx |  8 ++++++--
 2 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 4eb9a36..b5a813f 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1199,7 +1199,7 @@ retry:
 }
 
 /* Look up the -iscsi command-line option */
-static QemuOpts *find_iscsi_opts(const char *id)
+static QemuOpts *find_iscsi_opts(const char *id, bool default_to_first)
 {
     QemuOptsList *list;
     QemuOpts *opts;
@@ -1210,13 +1210,9 @@ static QemuOpts *find_iscsi_opts(const char *id)
     }
 
     opts = qemu_opts_find(list, id);
-    if (opts == NULL) {
+    if (opts == NULL && default_to_first) {
         opts = QTAILQ_FIRST(&list->head);
-        if (!opts) {
-            return NULL;
-        }
     }
-
     return opts;
 }
 
@@ -1411,6 +1407,11 @@ static QemuOptsList runtime_opts = {
             .type = QEMU_OPT_STRING,
             .help = "URL to the iscsi image",
         },
+        {
+            .name = "iscsi",
+            .type = QEMU_OPT_STRING,
+            .help = "id of iscsi object",
+        },
         { /* end of list */ }
     },
 };
@@ -1550,6 +1551,7 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     QemuOpts *iscsi_opts;
     Error *local_err = NULL;
     const char *filename;
+    const char *iscsi_opts_id;
     int i, ret = 0, timeout = 0;
 
     opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
@@ -1571,7 +1573,12 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
 
     memset(iscsilun, 0, sizeof(IscsiLun));
 
-    iscsi_opts = find_iscsi_opts(iscsi_url->target);
+    iscsi_opts_id = qemu_opt_get(opts, "iscsi");
+    if (iscsi_opts_id) {
+        iscsi_opts = find_iscsi_opts(iscsi_opts_id, false);
+    } else {
+        iscsi_opts = find_iscsi_opts(iscsi_url->target, true);
+    }
 
     initiator_name = parse_initiator_name(iscsi_opts);
 
diff --git a/qemu-options.hx b/qemu-options.hx
index a71aaf8..89da93d 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2515,14 +2515,18 @@ LIBISCSI_CHAP_PASSWORD="password" \
 qemu-system-i386 -drive file=iscsi://192.0.2.1/iqn.2001-04.com.example/1
 @end example
 
+An iSCSI drive is associated with an -iscsi option as follows.  If -drive
+...,iscsi=<iscsi-id> is present then the -iscsi option with that id is used.
+Otherwise an -iscsi option whose id matches the drive's target IQN is searched
+and if the search fails the first -iscsi option is used.
+
 iSCSI support is an optional feature of QEMU and only available when
 compiled and linked against libiscsi.
 ETEXI
 DEF("iscsi", HAS_ARG, QEMU_OPTION_iscsi,
     "-iscsi [user=user][,password=password]\n"
     "       [,header-digest=CRC32C|CR32C-NONE|NONE-CRC32C|NONE\n"
-    "       [,initiator-name=initiator-iqn][,id=target-iqn]\n"
-    "       [,timeout=timeout]\n"
+    "       [,initiator-name=initiator-iqn][,id=id][,timeout=timeout]\n"
     "                iSCSI session parameters\n", QEMU_ARCH_ALL)
 STEXI
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH 0/3] iscsi: add -drive ..., iscsi=<iscsi-id> parameter

[Daniel P. Berrange]

On Wed, Sep 07, 2016 at 05:23:09PM -0400, Stefan Hajnoczi wrote:
> Add a parameter to associate an iSCSI block driver instance with an -iscsi
> object.  This is more powerful than relying on the implicit target IQN naming
> convention since that only allows one -iscsi object per target.

I'm inclined to say we should *not* do this, as it just serves to perpetuate
usage of the insane -iscsi arg. Instead we could focus on fixing the iSCSI
blockdev for 2.8 so that it directly supports properties for everything that
you can set via -iscsi and deprecate -iscsi entirely. We've got enough time
during 2.8 to get this entirely converted I think.

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 0/3] iscsi: add -drive ..., iscsi=<iscsi-id> parameter

[Stefan Hajnoczi]

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

On Thu, Sep 08, 2016 at 09:14:10AM +0100, Daniel P. Berrange wrote:
> On Wed, Sep 07, 2016 at 05:23:09PM -0400, Stefan Hajnoczi wrote:
> > Add a parameter to associate an iSCSI block driver instance with an -iscsi
> > object.  This is more powerful than relying on the implicit target IQN naming
> > convention since that only allows one -iscsi object per target.
> 
> I'm inclined to say we should *not* do this, as it just serves to perpetuate
> usage of the insane -iscsi arg. Instead we could focus on fixing the iSCSI
> blockdev for 2.8 so that it directly supports properties for everything that
> you can set via -iscsi and deprecate -iscsi entirely. We've got enough time
> during 2.8 to get this entirely converted I think.

That's fine.  There's not much advantage to defining -iscsi objects
except that a single -iscsi can be used by multiple iscsi drives.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]