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 <berra...@redhat.com> > --- >
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); >
