Re: [libvirt] [PATCH v2 8/8] hostdev: Add iSCSI hostdev XML
On 08/05/2014 06:00 AM, John Ferlan wrote:
Maybe perhaps sleep helped me figure this out... Is the following what
you had in mind? (and it passes make check too)
<define name="hostdevsubsysscsi">
<attribute name="type">
<value>scsi</value>
</attribute>
<optional>
<attribute name="sgio">
<choice>
<value>filtered</value>
<value>unfiltered</value>
</choice>
</attribute>
</optional>
<choice>
<group>
<element name="source">
<interleave>
<ref name="sourceinfoadapter"/>
<element name="address">
<ref name="scsiaddress"/>
</element>
</interleave>
</element>
</group>
Looks better - this says that the caller that uses hostdevsubsysscsi
either has sourceinfoadapter (with no auth, and no protocol)...
<group>
<optional>
<ref name='diskAuth'/>
</optional>
<element name="source">
<attribute name="protocol">
<choice>
<value>iscsi</value>
</choice>
</attribute>
<attribute name="name">
<text/>
</attribute>
<interleave>
<oneOrMore>
<element name='host'>
<attribute name='name'>
<text/>
</attribute>
<optional>
<attribute name='port'>
<ref name="PortNumber"/>
</attribute>
</optional>
<empty/>
</element>
</oneOrMore>
</interleave>
</element>
</group>
or <source protocol='iscsi'> with optional diskAuth.
It still looks a bit odd, though. Normally, when we have two different
<source> children, the parent element contains an attribute that says
which child syntax we will be looking for. Something like:
<define name="hostdevsubsysscsi">
<attribute name="type">
<value>scsi</value>
</attribute>
<choice>
<group> <!-- default to adapter -->
<optional>
<attribute name='???'>
<value>adapter</value>
</attribute>
</optional>
<element name='source'>
<ref name="sourceinfoadapter"/>
</element>
</group>
<group> <!-- new code -->
<attribute name='???'>
<value>iscsi</value>
</attribute>
<element name="source">
<attribute name="protocol">
<value>iscsi</value>
</attribute>
<attribute name="name">
<text/>
</attribute>
...
but in writing that, I don't know what to call the new attribute (the
name='???' above), since we already have type='scsi'. Does that mean we
need TWO types, type='scsi' for the old code, and type='iscsi' for the
new additions?
I'm typing this email without looking at the full context of the patch;
maybe seeing it inline with the added test cases will help make it
obvious how we know which style of <source> is in use.
I suppose I could/should follow the disk and put auth after the
source...
Yeah. The <interleave> will let it come first when the user writes it
that way, but for readability, output auth after source makes more sense.
Would you like to see a v3? also including the extraneous spacing
change from the initial review of 5/8 and I had created a separate patch
on (which wasn't technically acked, so I didn't push):
http://www.redhat.com/archives/libvir-list/2014-July/msg01268.html
Yes, I think that's probably wise.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 539 bytes)