| Message ID | 1460557100-1393-1-git-send-email-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wednesday 13 April 2016 15:18:20 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. Maybe it would be worth as workaround for 2.6, although ... > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > --- > > 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 > > block/iscsi.c | 21 ++++++++++++++++++++- > 1 file changed, 20 insertions(+), 1 deletion(-) > > diff --git a/block/iscsi.c b/block/iscsi.c > index 302baf8..7475cc9 100644 > --- a/block/iscsi.c > +++ b/block/iscsi.c > @@ -1070,6 +1070,22 @@ retry: > return 0; > } > > + > +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 *target, > Error **errp) > { > @@ -1079,13 +1095,16 @@ static void parse_chap(struct iscsi_context *iscsi, const char *target, > const char *password = NULL; > const char *secretid; > char *secret = NULL; > + char *targetid = NULL; > > list = qemu_find_opts("iscsi"); > if (!list) { > return; > } > > - opts = qemu_opts_find(list, target); > + targetid = convert_target_to_id(target); > + opts = qemu_opts_find(list, targetid); > + g_free(targetid); > if (opts == NULL) { > opts = QTAILQ_FIRST(&list->head); > if (!opts) { ... the commit message seems to suggest that it applies to all the iSCSI parameter, but it actually works on the authentication-related ones. IMHO a better way would be using convert_target_to_id directly in iscsi_open on iscsi_url->target (right after the url parsing) passing the converted id to parse_initiator_name, iscsi_set_targetname, parse_chap, and parse_header_digest: this way it can apply to all the parameters. Thanks,
diff --git a/block/iscsi.c b/block/iscsi.c index 302baf8..7475cc9 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -1070,6 +1070,22 @@ retry: return 0; } + +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 *target, Error **errp) { @@ -1079,13 +1095,16 @@ static void parse_chap(struct iscsi_context *iscsi, const char *target, const char *password = NULL; const char *secretid; char *secret = NULL; + char *targetid = NULL; list = qemu_find_opts("iscsi"); if (!list) { return; } - opts = qemu_opts_find(list, target); + targetid = convert_target_to_id(target); + opts = qemu_opts_find(list, targetid); + g_free(targetid); if (opts == NULL) { opts = QTAILQ_FIRST(&list->head); if (!opts) {
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 <berrange@redhat.com> --- 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 block/iscsi.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)