Re: [libvirt] [PATCH v2 8/8] hostdev: Add iSCSI hostdev XML

5 Aug 2014

      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