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

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 539 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140805/a50dbcfb/attachment-0001.sig>