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

[John Ferlan]

On 08/05/2014 11:20 AM, Eric Blake wrote:
> 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?

Both are 'scsi' adapters - existing is 'scsi_host' and the new one is 'iscsi'.
I'm unsure about putting the 'optional' attribute name really does. Not to
confuse things further, but the next step in the evolution of this is to add
a fiber channel adapter (eg, fc_host).

The "first" format is for the "scsi_host" with the valid XML being:

    <hostdev mode='subsystem' type='scsi'>
      <source>
        <adapter name='scsi_host0'/>
        <address type='scsi' bus='0' target='0' unit='0'/>
      </source>
      <readonly/>
      <address type='drive' controller='0' bus='0' target='0' unit='0'/>
    </hostdev>


The "second" format is for the "iscsi" valid XML for 'iscsi' is:

    <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>


I'll send a v3 shortly


John
> 
> 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.
>