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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list