[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On 07/21/2014 02:47 PM, John Ferlan wrote:
> Introduce a new structure to handle an iSCSI host device based on the
> existing virDomainHostdevSubsysSCSI by adding a "protocol='iscsi'" to
> the <source/> element.  The hostdev structure mimics the existing
> <disk/> element for an iSCSI device (network) device. New XML is:
> 
>   <hostdev mode='subsystem' type='scsi' managed='yes'>
>     <auth username='myname'>
>       <secret type='iscsi' usage='mycluster_myname'/>
>     </auth>
>     <source protocol='iscsi' name='iqn.1992-01.com.example'>
>       <host name='example.org' port='3260'/>
>     </source>
>     <address type='drive' controller='0' bus='0' target='2' unit='5'/>
>   </hostdev>
> 
> The controller element will mimic the existing scsi_host code insomuch
> as when 'lsi' and 'virtio-scsi' are used.
> 
> Signed-off-by: John Ferlan <jferlan redhat com>
> ---
>  docs/formatdomain.html.in                          | 142 ++++++++++++-------
>  docs/schemas/domaincommon.rng                      |  46 ++++++-
>  src/conf/domain_conf.c                             | 152 ++++++++++++++++++---
>  .../qemuxml2argv-hostdev-scsi-lsi-iscsi-auth.args  |  14 ++
>  .../qemuxml2argv-hostdev-scsi-lsi-iscsi-auth.xml   |  46 +++++++
>  .../qemuxml2argv-hostdev-scsi-lsi-iscsi.args       |  14 ++
>  .../qemuxml2argv-hostdev-scsi-lsi-iscsi.xml        |  40 ++++++
>  ...emuxml2argv-hostdev-scsi-virtio-iscsi-auth.args |  16 +++
>  ...qemuxml2argv-hostdev-scsi-virtio-iscsi-auth.xml |  46 +++++++
>  .../qemuxml2argv-hostdev-scsi-virtio-iscsi.args    |  16 +++
>  .../qemuxml2argv-hostdev-scsi-virtio-iscsi.xml     |  40 ++++++
>  tests/qemuxml2argvtest.c                           |  16 +++
>  tests/qemuxml2xmltest.c                            |   5 +
>  13 files changed, 524 insertions(+), 69 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-lsi-iscsi-auth.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-lsi-iscsi-auth.xml
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-lsi-iscsi.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-lsi-iscsi.xml
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth.xml
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi.xml
> 

> +          <dt>usb</dt>
> +          <dd>For USB devices, the user is responsible to call
> +            <code>virNodeDeviceDettach</code> (or
> +            <code>virsh nodedev-dettach</code>) before starting the guest

The (preferred) virsh command is nodedev-detach (we can't correct the
API typo, but did correct the virsh typo).


> +          <dt>scsi</dt>
> +          <dd>SCSI devices are described by both the <code>adapter</code>
> +            and <code>address</code> elements.
> +            <p>
> +            <span class="since">Since 1.2.7</span>, the <code>source</code>

1.2.8 now.


> +++ b/docs/schemas/domaincommon.rng
> @@ -3563,13 +3563,47 @@
>          </choice>
>        </attribute>
>      </optional>
> +    <optional>
> +      <ref name='diskAuth'/>
> +    </optional>

Does diskAuth work for all configurations, or only for the new iscsi
configuration?  By putting it here, you are allowing it for all users,...

>      <element name="source">
> -      <interleave>
> -        <ref name="sourceinfoadapter"/>
> -        <element name="address">
> -          <ref name="scsiaddress"/>
> -        </element>
> -      </interleave>
> +      <optional>
> +        <attribute name="protocol">
> +          <choice>
> +            <value>iscsi</value>
> +          </choice>

...whereas using some <group> magic could make it valid only when
protocol is iscsi.

> +        </attribute>
> +        <attribute name="name">
> +          <text/>
> +        </attribute>
> +      </optional>
> +      <choice>
> +        <group>
> +          <interleave>
> +            <ref name="sourceinfoadapter"/>
> +            <element name="address">
> +              <ref name="scsiaddress"/>
> +            </element>
> +          </interleave>
> +        </group>
> +        <group>
> +          <interleave>
> +            <oneOrMore>
> +              <element name='host'>
> +                <attribute name='name'>
> +                  <text/>
> +                </attribute>
> +                <optional>
> +                  <attribute name='port'>
> +                    <ref name="PortNumber"/>
> +                  </attribute>
> +                </optional>
> +                <empty/>
> +              </element>
> +            </oneOrMore>
> +          </interleave>

This <interleave> adds nothing.  It doesn't hurt to leave it in, but
when there is only one possible (repetition of a) child "host"
<element>, there is nothing to be interleaved.


> @@ -4205,10 +4210,93 @@ virDomainHostdevSubsysSCSIHostDefParseXML(xmlNodePtr sourcenode,
>  }
>  
>  static int
> +virDomainHostdevSubsysSCSIiSCSIDefParseXML(xmlNodePtr sourcenode,
> +                                           xmlNodePtr authnode,
> +                                           virDomainHostdevSubsysSCSIPtr def)
> +{
> +    int ret = -1;
> +    int auth_secret_usage = -1;
> +    virStorageAuthDefPtr authdef = NULL;
> +    virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = &def->u.iscsi;
> +
> +    /* Similar to virDomainDiskSourceParse for a VIR_STORAGE_TYPE_NETWORK */

Is there enough commonality to be worth a common shared function?  I'm
okay if there isn't, but want to make sure we aren't doing unneeded
duplication.


> +    }
> +    if (iscsisrc->nhosts > 1) {
> +        virReportError(VIR_ERR_XML_ERROR, "%s",
> +                       _("only one source host address may be specified "
> +                         "for the iSCSI hostdev"));
> +        goto cleanup;

This doesn't match the .rng file which said <oneOrMore>.


> +static int
>  virDomainHostdevSubsysSCSIDefParseXML(xmlNodePtr sourcenode,
> +                                      xmlNodePtr authnode,
>                                        virDomainHostdevSubsysSCSIPtr scsisrc)
>  {
> -    return virDomainHostdevSubsysSCSIHostDefParseXML(sourcenode, scsisrc);
> +    char *protocol = NULL;
> +    int ret = -1;
> +
> +    if ((protocol = virXMLPropString(sourcenode, "protocol"))) {
> +        scsisrc->protocol =
> +            virDomainHostdevSubsysSCSIProtocolTypeFromString(protocol);
> +        if (scsisrc->protocol < 0) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("Unknown SCSI subsystem protcol '%s'"),

s/protcol/protocol/

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]