[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 for-2.6] block: convert iscsi target to a val
From: |
John Ferlan |
Subject: |
Re: [Qemu-devel] [PATCH v2 for-2.6] block: convert iscsi target to a valid ID for -iscsi arg lookup |
Date: |
Wed, 13 Apr 2016 21:41:54 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 |
On 04/13/2016 12:17 PM, Daniel P. Berrange wrote:
> 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>
> ---
>
Figured I'd chime in since I tripped across this today...
I think the one thing that concerns me about this '_' approach is we'd
be "stuck" with it. Eventually if 'initiator-name' is added to the
-drive command (as well as being able to parse the 'user=' and
'password-secret='), then needing to add -iscsi wouldn't be required for
libvirt. Whether this patch would be required after -drive is modified
was the other question rattling around. So I figured I'd see if I can
get things to work without it...
Using the v1 of this patch series did work for libvirt if I passed the
"id=" shown above using the '_' instead of ':'; however, taking the Pino
Toscano's series in mind, I can also start a domain using the
"initiator-name=" and "id=" parameters for '-iscsi' as follows:
...
-object
secret,id=masterKey0,format=raw,file=/var/lib/libvirt/qemu/domain-3-jaf/master-key.aes
...
-iscsi
id=iscsi-chap-netpool,initiator-name=iqn.2013-12.com.example,user=redhat,password-secret=virtio-disk1-ivKey0
-drive
file=iscsi://192.168.122.1:3260/iqn.2013-12.com.example%3Aiscsi-chap-netpool/1,format=raw,if=none,id=drive-virtio-disk1
-device
virtio-blk-pci,scsi=off,bus=pci.0,addr=0x9,drive=drive-virtio-disk1,id=virtio-disk1,bootindex=1
...
So without this patch - I should be able to get things to work starting
a domain. Long ago I tagged a libvir-list posting from Rich Jones asking
about using the '-iscsi initiator-name=' command:
http://www.redhat.com/archives/libvir-list/2014-July/msg00281.html
Since what libvirt was using worked, I just left it as one of those
someday I'll understand how/if it can be used...
John
> 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);
>