| Message ID | 2757099.oMy77gOrqp@pendragon.usersys.redhat.com |
|---|---|
| State | New |
| Headers | show |
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 \ ?
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 <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.
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
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; } }
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 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,