[Qemu-devel] iSCSI options for IQN with colons

[Qemu-devel] iSCSI options for IQN with colons

[Pino Toscano]

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

Hi,

while testing the integration of QEMU with iSCSI, I was setting up an
environment with both target and initiator IQNs with colons. Then I
tried to connect to two different targets using two different initiator
IQN, like the following:

  $ qemu ... \
      -iscsi id=iqn.2015-11.com.bla:suffix1,initiator-name=iqn.2015-11.com.bla:suffix1-initiator \
      -iscsi id=iqn.2015-11.com.bla:suffix2,initiator-name=iqn.2015-11.com.bla:suffix2-initiator \
      -drive file=iscsi://server/iqn.2015-11.com.bla%3Asuffix1/0,format=raw,id=hd1,if=none \
      -drive file=iscsi://server/iqn.2015-11.com.bla%3Asuffix2/0,format=raw,id=hd2,if=none \
      ...

which didn't work at first:

  qemu-system-x86_64: -iscsi id=iqn.2015-11.com.bla:suffix1,initiator-name=iqn.2015-11.com.bla:suffix1-initiator: Parameter 'id' expects an identifier

which, according to id_wellformed in id.c, is true. Allowing colons in
id=... like in the following patch

diff --git a/util/id.c b/util/id.c
index bcc64d8..25fca9d 100644
--- a/util/id.c
+++ b/util/id.c
@@ -20,7 +20,7 @@ bool id_wellformed(const char *id)
         return false;
     }
     for (i = 1; id[i]; i++) {
-        if (!qemu_isalnum(id[i]) && !strchr("-._", id[i])) {
+        if (!qemu_isalnum(id[i]) && !strchr("-._:", id[i])) {
             return false;
         }
     }

allowed me to work run QEMU with the attached disks.

The question basically boils down to whether it is right to reject
colons in id:
- if so, then there should be a way to allow them only in id of -iscsi
  (since colons can be part of IQNs)
- if not, whether allowing them could cause regressions in option
  parsing

Thanks,
-- 
Pino Toscano

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

Re: [Qemu-devel] iSCSI options for IQN with colons

[Markus Armbruster]

Beware, I know next to nothing about iSCSI.

Pino Toscano <ptoscano@redhat.com> writes:

> Hi,
>
> while testing the integration of QEMU with iSCSI, I was setting up an
> environment with both target and initiator IQNs with colons. Then I
> tried to connect to two different targets using two different initiator
> IQN, like the following:
>
>   $ qemu ... \
>       -iscsi id=iqn.2015-11.com.bla:suffix1,initiator-name=iqn.2015-11.com.bla:suffix1-initiator \
>       -iscsi id=iqn.2015-11.com.bla:suffix2,initiator-name=iqn.2015-11.com.bla:suffix2-initiator \
>       -drive file=iscsi://server/iqn.2015-11.com.bla%3Asuffix1/0,format=raw,id=hd1,if=none \
>       -drive file=iscsi://server/iqn.2015-11.com.bla%3Asuffix2/0,format=raw,id=hd2,if=none \
>       ...
>
> which didn't work at first:
>
>   qemu-system-x86_64: -iscsi id=iqn.2015-11.com.bla:suffix1,initiator-name=iqn.2015-11.com.bla:suffix1-initiator: Parameter 'id' expects an identifier
>
> which, according to id_wellformed in id.c, is true. Allowing colons in
> id=... like in the following patch
>
> diff --git a/util/id.c b/util/id.c
> index bcc64d8..25fca9d 100644
> --- a/util/id.c
> +++ b/util/id.c
> @@ -20,7 +20,7 @@ bool id_wellformed(const char *id)
>          return false;
>      }
>      for (i = 1; id[i]; i++) {
> -        if (!qemu_isalnum(id[i]) && !strchr("-._", id[i])) {
> +        if (!qemu_isalnum(id[i]) && !strchr("-._:", id[i])) {
>              return false;
>          }
>      }
>
> allowed me to work run QEMU with the attached disks.
>
> The question basically boils down to whether it is right to reject
> colons in id:
> - if so, then there should be a way to allow them only in id of -iscsi
>   (since colons can be part of IQNs)
> - if not, whether allowing them could cause regressions in option
>   parsing

Have you tried

    -iscsi id=iscsi.1,initiator-name=iqn.2015-11.com.bla:suffix1-initiator \
    -iscsi id=iscsi.2,initiator-name=iqn.2015-11.com.bla:suffix2-initiator \
    -drive file=iscsi://server/iqn.2015-11.com.bla%3Asuffix1/0,format=raw,id=hd1,if=none \
    -drive file=iscsi://server/iqn.2015-11.com.bla%3Asuffix2/0,format=raw,id=hd2,if=none \

?

Re: [Qemu-devel] iSCSI options for IQN with colons

[Pino Toscano]

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

On Tuesday 01 December 2015 10:27:28 Markus Armbruster wrote:
> Beware, I know next to nothing about iSCSI.
> 
> Pino Toscano <ptoscano@redhat.com> writes:
> 
> > Hi,
> >
> > while testing the integration of QEMU with iSCSI, I was setting up an
> > environment with both target and initiator IQNs with colons. Then I
> > tried to connect to two different targets using two different initiator
> > IQN, like the following:
> >
> >   $ qemu ... \
> >       -iscsi id=iqn.2015-11.com.bla:suffix1,initiator-name=iqn.2015-11.com.bla:suffix1-initiator \
> >       -iscsi id=iqn.2015-11.com.bla:suffix2,initiator-name=iqn.2015-11.com.bla:suffix2-initiator \
> >       -drive file=iscsi://server/iqn.2015-11.com.bla%3Asuffix1/0,format=raw,id=hd1,if=none \
> >       -drive file=iscsi://server/iqn.2015-11.com.bla%3Asuffix2/0,format=raw,id=hd2,if=none \
> >       ...
> >
> > which didn't work at first:
> >
> >   qemu-system-x86_64: -iscsi id=iqn.2015-11.com.bla:suffix1,initiator-name=iqn.2015-11.com.bla:suffix1-initiator: Parameter 'id' expects an identifier
> >
> > which, according to id_wellformed in id.c, is true. Allowing colons in
> > id=... like in the following patch
> >
> > diff --git a/util/id.c b/util/id.c
> > index bcc64d8..25fca9d 100644
> > --- a/util/id.c
> > +++ b/util/id.c
> > @@ -20,7 +20,7 @@ bool id_wellformed(const char *id)
> >          return false;
> >      }
> >      for (i = 1; id[i]; i++) {
> > -        if (!qemu_isalnum(id[i]) && !strchr("-._", id[i])) {
> > +        if (!qemu_isalnum(id[i]) && !strchr("-._:", id[i])) {
> >              return false;
> >          }
> >      }
> >
> > allowed me to work run QEMU with the attached disks.
> >
> > The question basically boils down to whether it is right to reject
> > colons in id:
> > - if so, then there should be a way to allow them only in id of -iscsi
> >   (since colons can be part of IQNs)
> > - if not, whether allowing them could cause regressions in option
> >   parsing
> 
> Have you tried
> 
>     -iscsi id=iscsi.1,initiator-name=iqn.2015-11.com.bla:suffix1-initiator \
>     -iscsi id=iscsi.2,initiator-name=iqn.2015-11.com.bla:suffix2-initiator \
>     -drive file=iscsi://server/iqn.2015-11.com.bla%3Asuffix1/0,format=raw,id=hd1,if=none \
>     -drive file=iscsi://server/iqn.2015-11.com.bla%3Asuffix2/0,format=raw,id=hd2,if=none \

This won't work, as the various parse_* in iscsi.c (e.g.
parse_initiator_name for the above cases) use the target IQN as
identifier for the parameters.

-- 
Pino Toscano

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

Re: [Qemu-devel] iSCSI options for IQN with colons

[Markus Armbruster]

Pino Toscano <ptoscano@redhat.com> writes:

> On Tuesday 01 December 2015 10:27:28 Markus Armbruster wrote:
>> Beware, I know next to nothing about iSCSI.
>> 
>> Pino Toscano <ptoscano@redhat.com> writes:
>> 
>> > Hi,
>> >
>> > while testing the integration of QEMU with iSCSI, I was setting up an
>> > environment with both target and initiator IQNs with colons. Then I
>> > tried to connect to two different targets using two different initiator
>> > IQN, like the following:
>> >
>> >   $ qemu ... \
>> >       -iscsi id=iqn.2015-11.com.bla:suffix1,initiator-name=iqn.2015-11.com.bla:suffix1-initiator \
>> >       -iscsi id=iqn.2015-11.com.bla:suffix2,initiator-name=iqn.2015-11.com.bla:suffix2-initiator \
>> >       -drive file=iscsi://server/iqn.2015-11.com.bla%3Asuffix1/0,format=raw,id=hd1,if=none \
>> >       -drive file=iscsi://server/iqn.2015-11.com.bla%3Asuffix2/0,format=raw,id=hd2,if=none \
>> >       ...
>> >
>> > which didn't work at first:
>> >
>> >   qemu-system-x86_64: -iscsi id=iqn.2015-11.com.bla:suffix1,initiator-name=iqn.2015-11.com.bla:suffix1-initiator: Parameter 'id' expects an identifier
>> >
>> > which, according to id_wellformed in id.c, is true. Allowing colons in
>> > id=... like in the following patch
>> >
>> > diff --git a/util/id.c b/util/id.c
>> > index bcc64d8..25fca9d 100644
>> > --- a/util/id.c
>> > +++ b/util/id.c
>> > @@ -20,7 +20,7 @@ bool id_wellformed(const char *id)
>> >          return false;
>> >      }
>> >      for (i = 1; id[i]; i++) {
>> > -        if (!qemu_isalnum(id[i]) && !strchr("-._", id[i])) {
>> > +        if (!qemu_isalnum(id[i]) && !strchr("-._:", id[i])) {
>> >              return false;
>> >          }
>> >      }
>> >
>> > allowed me to work run QEMU with the attached disks.
>> >
>> > The question basically boils down to whether it is right to reject
>> > colons in id:
>> > - if so, then there should be a way to allow them only in id of -iscsi
>> >   (since colons can be part of IQNs)
>> > - if not, whether allowing them could cause regressions in option
>> >   parsing
>> 
>> Have you tried
>> 
>>     -iscsi id=iscsi.1,initiator-name=iqn.2015-11.com.bla:suffix1-initiator \
>>     -iscsi id=iscsi.2,initiator-name=iqn.2015-11.com.bla:suffix2-initiator \
>>     -drive
>> file=iscsi://server/iqn.2015-11.com.bla%3Asuffix1/0,format=raw,id=hd1,if=none
>> \
>>     -drive
>> file=iscsi://server/iqn.2015-11.com.bla%3Asuffix2/0,format=raw,id=hd2,if=none
>> \
>
> This won't work, as the various parse_* in iscsi.c (e.g.
> parse_initiator_name for the above cases) use the target IQN as
> identifier for the parameters.

Option parameter "id" is for naming things so that other things can
refer to them.  The actual name should not matter.  If it does, it's a
bug.

>From my largely iSCSI-ignorant point of view, it looks like -drive
file=iscsi *might* use the target IQN it gets from the URL to look up
the matching -iscsi option.  That would be inappropriate.

I'm copying iSCSI maintainers for an authoritative answer.

Re: [Qemu-devel] iSCSI options for IQN with colons

[Paolo Bonzini]

On 01/12/2015 14:43, Markus Armbruster wrote:
> Option parameter "id" is for naming things so that other things can
> refer to them.  The actual name should not matter.  If it does, it's a
> bug.
> 
> From my largely iSCSI-ignorant point of view, it looks like -drive
> file=iscsi *might* use the target IQN it gets from the URL to look up
> the matching -iscsi option.  That would be inappropriate.

Yes, it does.

I'm not sure if it's inappropriate; "id" is the one mechanism that
QemuOpts provides for looking up things, and it makes sense to use it
even if the lookup key is not user-controlled.

Unfortunately, the limitations on ids make -iscsi almost unusable; IQNs
almost always have a colon (the syntax is
iqn.YYYY-MM.com.example:string.controlled.by.example.com.owner; the
after-colon part is optional but in practice will be there).  Either we
fix it with Pino's patch, or we might as well remove it.

In 2.6 we probably should get the new secret API, and -iscsi should be
modified so that you can specify a reference to a secret directly in -drive.

Paolo