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

[Pino Toscano <ptoscano@redhat.com>]

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

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,
-- 
Pino Toscano

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]