4:21 p.m.
On 08/04/2014 02:21 PM, Eric Blake wrote:
On 07/21/2014 02:47 PM, John Ferlan wrote:
> Introduce a new structure to handle an iSCSI host device based on the
> existing virDomainHostdevSubsysSCSI by adding a "protocol='iscsi'"
to
> the <source/> element. The hostdev structure mimics the existing
> <disk/> element for an iSCSI device (network) device. New XML is:
>
> <hostdev mode='subsystem' type='scsi' managed='yes'>
> <auth username='myname'>
> <secret type='iscsi' usage='mycluster_myname'/>
> </auth>
> <source protocol='iscsi' name='iqn.1992-01.com.example'>
> <host name='example.org' port='3260'/>
> </source>
> <address type='drive' controller='0' bus='0'
target='2' unit='5'/>
> </hostdev>
>
> The controller element will mimic the existing scsi_host code insomuch
> as when 'lsi' and 'virtio-scsi' are used.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> docs/formatdomain.html.in | 142 ++++++++++++-------
> docs/schemas/domaincommon.rng | 46 ++++++-
> src/conf/domain_conf.c | 152 ++++++++++++++++++---
> .../qemuxml2argv-hostdev-scsi-lsi-iscsi-auth.args | 14 ++
> .../qemuxml2argv-hostdev-scsi-lsi-iscsi-auth.xml | 46 +++++++
> .../qemuxml2argv-hostdev-scsi-lsi-iscsi.args | 14 ++
> .../qemuxml2argv-hostdev-scsi-lsi-iscsi.xml | 40 ++++++
> ...emuxml2argv-hostdev-scsi-virtio-iscsi-auth.args | 16 +++
> ...qemuxml2argv-hostdev-scsi-virtio-iscsi-auth.xml | 46 +++++++
> .../qemuxml2argv-hostdev-scsi-virtio-iscsi.args | 16 +++
> .../qemuxml2argv-hostdev-scsi-virtio-iscsi.xml | 40 ++++++
> tests/qemuxml2argvtest.c | 16 +++
> tests/qemuxml2xmltest.c | 5 +
> 13 files changed, 524 insertions(+), 69 deletions(-)
> create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-lsi-iscsi-auth.args
> create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-lsi-iscsi-auth.xml
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-lsi-iscsi.args
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-lsi-iscsi.xml
> create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth.args
> create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth.xml
> create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi.args
> create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi.xml
>
> + <dt>usb</dt>
> + <dd>For USB devices, the user is responsible to call
> + <code>virNodeDeviceDettach</code> (or
> + <code>virsh nodedev-dettach</code>) before starting the
guest
The (preferred) virsh command is nodedev-detach (we can't correct the
API typo, but did correct the virsh typo).
Fixed (plus another such typo)
> + <dt>scsi</dt>
> + <dd>SCSI devices are described by both the
<code>adapter</code>
> + and <code>address</code> elements.
> + <p>
> + <span class="since">Since 1.2.7</span>, the
<code>source</code>
1.2.8 now.
Updated...
> +++ b/docs/schemas/domaincommon.rng
> @@ -3563,13 +3563,47 @@
> </choice>
> </attribute>
> </optional>
> + <optional>
> + <ref name='diskAuth'/>
> + </optional>
Does diskAuth work for all configurations, or only for the new iscsi
configuration? By putting it here, you are allowing it for all users,...
> <element name="source">
> - <interleave>
> - <ref name="sourceinfoadapter"/>
> - <element name="address">
> - <ref name="scsiaddress"/>
> - </element>
> - </interleave>
> + <optional>
> + <attribute name="protocol">
> + <choice>
> + <value>iscsi</value>
> + </choice>
...whereas using some <group> magic could make it valid only when
protocol is iscsi.
So clearly this RNG syntax is BFM and confusing...
The "goal" is/was to copy the same element logic from the disk element,
but unlike disk it will only be valid for iscsi. It should have the
following format:
<devices>
<hostdev mode='subsystem' type='scsi'>
<source protocol='iscsi'
name='iqn.2014-08.com.example:iscsi-nopool/1'>
<host name='example.com' port='3260'/>
</source>
<auth username='myuser'>
<secret type='iscsi' usage='libvirtiscsi'/>
</auth>
<address type='drive' controller='0' bus='0'
target='0' unit='0'/>
</hostdev>
</devices>
...
where source would be reqd, auth would be option
It's not very clear to me where/how to place it in the rng file...
> + </attribute>
> + <attribute name="name">
> + <text/>
> + </attribute>
> + </optional>
> + <choice>
> + <group>
> + <interleave>
> + <ref name="sourceinfoadapter"/>
> + <element name="address">
> + <ref name="scsiaddress"/>
> + </element>
> + </interleave>
> + </group>
> + <group>
> + <interleave>
> + <oneOrMore>
> + <element name='host'>
> + <attribute name='name'>
> + <text/>
> + </attribute>
> + <optional>
> + <attribute name='port'>
> + <ref name="PortNumber"/>
> + </attribute>
> + </optional>
> + <empty/>
> + </element>
> + </oneOrMore>
> + </interleave>
This <interleave> adds nothing. It doesn't hurt to leave it in, but
when there is only one possible (repetition of a) child "host"
<element>, there is nothing to be interleaved.
Like <group>, the <interleave> didn't quite make sense in how/when it
should be used... obviously :-)
I was trying to follow 'diskSource' and 'diskSourceNetwork' in order to
allow the definition of the <host name="string"
port="string/number">.
In any case, I went back and forth on this. On one hand, it's been
allowed by the disk syntax, while on the other hand it doesn't make
sense to allow more than one, but yet - I'm trying to mimic the disk
syntax. I think
> @@ -4205,10 +4210,93 @@
virDomainHostdevSubsysSCSIHostDefParseXML(xmlNodePtr sourcenode,
> }
>
> static int
> +virDomainHostdevSubsysSCSIiSCSIDefParseXML(xmlNodePtr sourcenode,
> + xmlNodePtr authnode,
> + virDomainHostdevSubsysSCSIPtr def)
> +{
> + int ret = -1;
> + int auth_secret_usage = -1;
> + virStorageAuthDefPtr authdef = NULL;
> + virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = &def->u.iscsi;
> +
> + /* Similar to virDomainDiskSourceParse for a VIR_STORAGE_TYPE_NETWORK */
Is there enough commonality to be worth a common shared function? I'm
okay if there isn't, but want to make sure we aren't doing unneeded
duplication.
I think I've already cut down as much as I can...
> + }
> + if (iscsisrc->nhosts > 1) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("only one source host address may be specified "
> + "for the iSCSI hostdev"));
> + goto cleanup;
This doesn't match the .rng file which said <oneOrMore>.
Right - so should I follow the logic of <disk> and allow the check in
the qemu code (qemuBuildNetworkDriveURI()) be the final arbiter? Yet
another struggle - where to cause failure...
I think the < 0 would have to stay since <host> would be required.
> +static int
> virDomainHostdevSubsysSCSIDefParseXML(xmlNodePtr sourcenode,
> + xmlNodePtr authnode,
> virDomainHostdevSubsysSCSIPtr scsisrc)
> {
> - return virDomainHostdevSubsysSCSIHostDefParseXML(sourcenode, scsisrc);
> + char *protocol = NULL;
> + int ret = -1;
> +
> + if ((protocol = virXMLPropString(sourcenode, "protocol"))) {
> + scsisrc->protocol =
> + virDomainHostdevSubsysSCSIProtocolTypeFromString(protocol);
> + if (scsisrc->protocol < 0) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("Unknown SCSI subsystem protcol
'%s'"),
s/protcol/protocol/
Fixed.
John