[Qemu-devel] [PATCH v2 for-2.6] block: convert iscsi target to a valid ID for -iscsi arg lookup

[Daniel P. Berrange]

The iSCSI block driver has a very strange approach whereby it
does not accept options directly as part of the -drive arg,
but instead takes them indirectly from a -iscsi arg. To make
up -driver and -iscsi args, it takes the iSCSI target name
and uses that as an ID value for the -iscsi arg lookup.

For example, given a -drive arg that looks like

 -drive 
file=iscsi://192.168.122.1:3260/iqn.2013-12.com.example%3Aiscsi-chap-netpool/1,format=raw,if=none,id=drive-virtio-disk0

The iSCSI driver will try to find the -iscsi arg with an
id of "iqn.2013-12.com.example:iscsi-chap-netpool" eg it
expects

  -iscsi 
id=iqn.2013-12.com.example:iscsi-chap-netpool,user=fred,password-secret=secret0

Unfortunately this is impossible to actually do in practice
because almost all iSCSI target names contain a ':' which
is not a valid ID character for QEMU:

 $ qemu-system-x86_64: -iscsi 
id=iqn.2013-12.com.example:iscsi-chap-netpool,user=redhat,password=123456: 
Parameter 'id' expects an identifier
 Identifiers consist of letters, digits, '-', '.', '_', starting with a letter.

So for this to work we need to remove the invalid characters
in some manner. This patch takes the simplest possible approach
of just converting all invalid characters into underscores. eg
you can now use

 $ qemu-system-x86_64: -iscsi 
id=iqn.2013-12.com.example_iscsi-chap-netpool,user=redhat,password=123456: 
Parameter 'id' expects an identifier

There is theoretically the possibility for collison with this
approach if there were 2 iSCSI luns attached to the same VM
with target names that clash on the escaped char: eg

  iqn.2013-12.com.example:iscsi-chap-netpool
  iqn.2013-12.com.example_iscsi-chap-netpool

but in reality this will never happen. In addition in QEMU 2.7
the need to use the -iscsi arg will be removed by allowing all
the desired options to be listed directly with -drive. IOW this
quick stripping of invalid characters is just a short term fix
that will ultimately go away. For this reason it is not thought
worthwhile to invent a full collision-free escaping syntax for
iSCSI target IDs.

Signed-off-by: Daniel P. Berrange <address@hidden>
---

Note this problem was previously raised:

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

and discussed the following month:

 http://lists.nongnu.org/archive/html/qemu-devel/2015-12/msg00112.html

Changed in v2:

 - Actually use the escaped target ID for all parameters,
   not just chap auth params (Pino)

 block/iscsi.c | 43 +++++++++++++++++++++++++++++++------------
 1 file changed, 31 insertions(+), 12 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 302baf8..8ee2e4d 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1070,7 +1070,23 @@ retry:
     return 0;
 }
 
-static void parse_chap(struct iscsi_context *iscsi, const char *target,
+
+static char *convert_target_to_id(const char *target)
+{
+    char *id = g_strdup(target);
+    size_t i;
+
+    for (i = 1; id[i]; i++) {
+        if (!qemu_isalnum(id[i]) && !strchr("-._", id[i])) {
+            id[i] = '_';
+        }
+    }
+
+    return id;
+}
+
+
+static void parse_chap(struct iscsi_context *iscsi, const char *targetid,
                        Error **errp)
 {
     QemuOptsList *list;
@@ -1085,7 +1101,7 @@ static void parse_chap(struct iscsi_context *iscsi, const 
char *target,
         return;
     }
 
-    opts = qemu_opts_find(list, target);
+    opts = qemu_opts_find(list, targetid);
     if (opts == NULL) {
         opts = QTAILQ_FIRST(&list->head);
         if (!opts) {
@@ -1123,7 +1139,7 @@ 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, const char 
*targetid,
                                 Error **errp)
 {
     QemuOptsList *list;
@@ -1135,7 +1151,7 @@ static void parse_header_digest(struct iscsi_context 
*iscsi, const char *target,
         return;
     }
 
-    opts = qemu_opts_find(list, target);
+    opts = qemu_opts_find(list, targetid);
     if (opts == NULL) {
         opts = QTAILQ_FIRST(&list->head);
         if (!opts) {
@@ -1161,7 +1177,7 @@ static void parse_header_digest(struct iscsi_context 
*iscsi, const char *target,
     }
 }
 
-static char *parse_initiator_name(const char *target)
+static char *parse_initiator_name(const char *targetid)
 {
     QemuOptsList *list;
     QemuOpts *opts;
@@ -1171,7 +1187,7 @@ static char *parse_initiator_name(const char *target)
 
     list = qemu_find_opts("iscsi");
     if (list) {
-        opts = qemu_opts_find(list, target);
+        opts = qemu_opts_find(list, targetid);
         if (!opts) {
             opts = QTAILQ_FIRST(&list->head);
         }
@@ -1195,7 +1211,7 @@ static char *parse_initiator_name(const char *target)
     return iscsi_name;
 }
 
-static int parse_timeout(const char *target)
+static int parse_timeout(const char *targetid)
 {
     QemuOptsList *list;
     QemuOpts *opts;
@@ -1203,7 +1219,7 @@ static int parse_timeout(const char *target)
 
     list = qemu_find_opts("iscsi");
     if (list) {
-        opts = qemu_opts_find(list, target);
+        opts = qemu_opts_find(list, targetid);
         if (!opts) {
             opts = QTAILQ_FIRST(&list->head);
         }
@@ -1453,6 +1469,7 @@ static int iscsi_open(BlockDriverState *bs, QDict 
*options, int flags,
     Error *local_err = NULL;
     const char *filename;
     int i, ret = 0, timeout = 0;
+    char *targetid = NULL;
 
     opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
     qemu_opts_absorb_qdict(opts, options, &local_err);
@@ -1473,7 +1490,8 @@ static int iscsi_open(BlockDriverState *bs, QDict 
*options, int flags,
 
     memset(iscsilun, 0, sizeof(IscsiLun));
 
-    initiator_name = parse_initiator_name(iscsi_url->target);
+    targetid = convert_target_to_id(iscsi_url->target);
+    initiator_name = parse_initiator_name(targetid);
 
     iscsi = iscsi_create_context(initiator_name);
     if (iscsi == NULL) {
@@ -1499,7 +1517,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, targetid, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
         ret = -EINVAL;
@@ -1515,7 +1533,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, targetid, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
         ret = -EINVAL;
@@ -1523,7 +1541,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(targetid);
 #if defined(LIBISCSI_API_VERSION) && LIBISCSI_API_VERSION >= 20150621
     iscsi_set_timeout(iscsi, timeout);
 #else
@@ -1643,6 +1661,7 @@ static int iscsi_open(BlockDriverState *bs, QDict 
*options, int flags,
 
 out:
     qemu_opts_del(opts);
+    g_free(targetid);
     g_free(initiator_name);
     if (iscsi_url != NULL) {
         iscsi_destroy_url(iscsi_url);
-- 
2.5.5