[libvirt] [PATCH v2 01/12] virstoragefile: Introduce virStoragePRDef

John Ferlan jferlan at redhat.com
Fri Mar 2 01:58:06 UTC 2018



On 02/21/2018 01:11 PM, Michal Privoznik wrote:
> This is a definition that holds information on SCSI persistent
> reservation settings. The XML part looks like this:
> 
>   <reservations enabled='yes' managed='no'>
>     <source type='unix' path='/path/to/qemu-pr-helper.sock' mode='client'/>
>   </reservations>
> 
> If @managed is set to 'yes' then the <source/> is not parsed.
> This design was agreed on here:
> 
> https://www.redhat.com/archives/libvir-list/2017-November/msg01005.html

The design may have been agreed upon, but sometimes once a bit of work
is done and the problem better understood the design can change.

I still don't understand what the purpose of enabled='no' would be.
Especially since when set to 'no', the <source> isn't formatted.

I'd prefer to see a bit more wordy commit to describe the functionality
and what it's used for... others, well I know, less is better because
it's all self documenting anyway...

In particular it's "confusing" what's done for when <source> is provided
and what is done when it's not.

> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  docs/formatdomain.html.in                          |  25 +++-
>  docs/schemas/domaincommon.rng                      |  34 +----
>  docs/schemas/storagecommon.rng                     |  50 +++++++
>  src/conf/domain_conf.c                             |  36 +++++
>  src/libvirt_private.syms                           |   3 +
>  src/util/virstoragefile.c                          | 148 +++++++++++++++++++++
>  src/util/virstoragefile.h                          |  15 +++
>  .../disk-virtio-scsi-reservations-not-managed.xml  |  40 ++++++
>  .../disk-virtio-scsi-reservations.xml              |  38 ++++++
>  .../disk-virtio-scsi-reservations-not-managed.xml  |   1 +
>  .../disk-virtio-scsi-reservations.xml              |   1 +
>  tests/qemuxml2xmltest.c                            |   4 +
>  12 files changed, 364 insertions(+), 31 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/disk-virtio-scsi-reservations-not-managed.xml
>  create mode 100644 tests/qemuxml2argvdata/disk-virtio-scsi-reservations.xml
>  create mode 120000 tests/qemuxml2xmloutdata/disk-virtio-scsi-reservations-not-managed.xml
>  create mode 120000 tests/qemuxml2xmloutdata/disk-virtio-scsi-reservations.xml
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 6fd2189cd..cbdc887a0 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -2563,7 +2563,10 @@
>    </disk>
>    <disk type='block' device='lun'>
>      <driver name='qemu' type='raw'/>
> -    <source dev='/dev/sda'/>
> +    <source dev='/dev/sda'>
> +      <reservations enabled='yes' managed='no'>

See note below w/r/t managed='no'

> +        <source type='unix' path='/path/to/qemu-pr-helper' mode='client'/>
> +      </reservations>
>      <target dev='sda' bus='scsi'/>
>      <address type='drive' controller='0' bus='0' target='3' unit='0'/>
>    </disk>
> @@ -2926,6 +2929,26 @@
>              <a href="formatstorageencryption.html">Storage Encryption</a>
>              page for more information.
>            </dd>
> +          <dt><code>reservations</code></dt>
> +          <dd><span class="since">Since libvirt 4.1.0</span>, the
> +            <code>reservations</code> can be a sub-element of the
> +            <code>source</code> element for storage sources (QEMU driver only).
> +            If present (and enabled) it enabled persistent reservations for
> +            SCSI based disks. The element has one mandatory attribute
> +            <code>enabled</code> with accepted values <code>yes</code> and
> +            <code>no</code>. If the feature is enabled, then there's another
> +            mandatory attribute <code>managed</code> (accepted values are the
> +            same as for <code>enabled</code>) that enables or disables libvirt
> +            spawning any helper process (should one be needed). However, if
> +            libvirt is not enabled spawning helper process (i.e. hypervisor
> +            should just use one which is already running), path to its socket
> +            must be provided in child element <code>source</code>, which
> +            currently accepts only the following attributes: <code>type</code>
> +            with one value <code>unix</code>, <code>path</code> with path the
> +            socket, and finally <code>mode</code> which accepts either
> +            <code>server</code> or <code>client</code> and specifies the role
> +            of hypervisor.
> +          </dd>

Since 4.2.0, the optional <code>reservations</code> sub-element of the
storage <code>source</code> element can be used to enable persistent
reservations for SCSI based disks (QEMU driver only). The element has
one mandatory attribute <code>managed</code> set to either 'yes' or
'no'. When <code>managed</code> is 'yes', libvirt will create a helper
process to manage the reservations and will check at restart for the
helper process. When <code>managed</code> is 'no', libvirt will provide
no management of the helper process. If libvirt is managing the helper
process, then the <code>source</code> sub-element must be provided with
mandatory attributes <code>type</code>, <code>path</code>, and
<code>mode</code>. The <code>type</code> attribute currently only
accepts 'unix' as an option and the <code>mode</code> attribute only
accepts 'client'. The <code>path</code> attribute supplies the path to
the UNIX socket for communication between libvirt and the helper process.


FWIW: What I wrote above is backwards from the implementation for
managed. At least it makes more sense to me. If libvirt is managing
something, then it will do the work, if libvirt is not managing
something then it doesn't do anything.  Think of how hostdev's are
managed or not. When managed, libvirt will detach and reattach. When not
managed the user is responsible to perform the detach and reattach.

BTW: While writing this I'm now wondering - so how does the managed='no'
path actually find or communicate with the helper process? Maybe that
answer comes later.

>          </dl>
>  
>          <p>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 8165e699d..198d4400b 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -1530,6 +1530,9 @@
>          <optional>
>            <ref name="encryption"/>
>          </optional>
> +        <optional>
> +          <ref name="reservations"/>
> +        </optional>
>          <zeroOrMore>
>            <ref name='devSeclabel'/>
>          </zeroOrMore>
> @@ -2431,18 +2434,6 @@
>        </attribute>
>      </optional>
>    </define>
> -  <define name="reconnect">
> -    <element name="reconnect">
> -      <attribute name="enabled">
> -        <ref name="virYesNo"/>
> -      </attribute>
> -      <optional>
> -        <attribute name="timeout">
> -          <ref name="unsignedInt"/>
> -        </attribute>
> -      </optional>
> -    </element>
> -  </define>

Why does this hunk move to storagecommon.rng? Since it's going to be
common between domain and storage (for vhostuser and persistent
reservations), maybe it should move to basictypes.rng.

>  
>    <!--
>        An interface description can either be of type bridge in which case
> @@ -2491,24 +2482,7 @@
>              <value>vhostuser</value>
>            </attribute>
>            <interleave>
> -            <element name="source">
> -              <attribute name="type">
> -                <value>unix</value>
> -              </attribute>
> -              <attribute name="path">
> -                <ref name="absFilePath"/>
> -              </attribute>
> -              <attribute name="mode">
> -                <choice>
> -                  <value>server</value>
> -                  <value>client</value>
> -                </choice>
> -              </attribute>
> -              <optional>
> -                 <ref name="reconnect"/>
> -              </optional>
> -              <empty/>
> -            </element>
> +            <ref name="unixSocketSource"/>
>              <ref name="interface-options"/>
>            </interleave>
>          </group>
> diff --git a/docs/schemas/storagecommon.rng b/docs/schemas/storagecommon.rng
> index edee1b084..eed0b3334 100644
> --- a/docs/schemas/storagecommon.rng
> +++ b/docs/schemas/storagecommon.rng
> @@ -39,6 +39,56 @@
>      </element>
>    </define>
>  

Perhaps all of the following should be in basictypes.rng?  Or some other
new common rng file...  Common between domain and storage where we seem
to be having more and more crossover.

> +  <define name="reconnect">
> +    <element name="reconnect">
> +      <attribute name="enabled">
> +        <ref name="virYesNo"/>
> +      </attribute>
> +      <optional>
> +        <attribute name="timeout">
> +          <ref name="unsignedInt"/>
> +        </attribute>
> +      </optional>
> +    </element>
> +  </define>
> +
> +  <define name='unixSocketSource'>
> +    <element name="source">
> +      <attribute name="type">
> +        <value>unix</value>
> +      </attribute>
> +      <attribute name="path">
> +        <ref name="absFilePath"/>
> +      </attribute>
> +      <attribute name="mode">
> +        <choice>
> +          <value>server</value>
> +          <value>client</value>
> +        </choice>
> +      </attribute>
> +      <optional>
> +         <ref name="reconnect"/>
> +      </optional>
> +      <empty/>
> +    </element>
> +  </define>
> +
> +  <define name='reservations'>
> +    <element name='reservations'>
> +      <attribute name='enabled'>
> +        <ref name='virYesNo'/>
> +      </attribute>
> +      <optional>
> +        <attribute name='managed'>
> +          <ref name='virYesNo'/>
> +        </attribute>
> +      </optional>> +      <optional>
> +        <ref name='unixSocketSource'/>

The parse/format won't allow the mode=server value nor will it do any
sort of reconnect...

> +      </optional>
> +    </element>
> +  </define>
> +
>    <define name='secret'>
>      <element name='secret'>
>        <attribute name='type'>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index f2ddde7a3..a1a6b0162 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5199,6 +5199,13 @@ virDomainDiskDefValidate(const virDomainDiskDef *disk)
>          }
>      }
>  
> +    if (disk->src->pr &&
> +        disk->device != VIR_DOMAIN_DISK_DEVICE_LUN) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("<reservations/> allowed only for lun disks"));
> +        return -1;
> +    }
> +>      /* Reject disks with a bus type that is not compatible with the
>       * given address type. The function considers only buses that are
>       * handled in common code. For other bus types it's not possible
> @@ -8613,6 +8620,29 @@ virDomainDiskSourcePrivateDataParse(xmlXPathContextPtr ctxt,
>  }
>  
>  
> +static int
> +virDomainDiskSourcePRParse(xmlNodePtr node,
> +                           virStoragePRDefPtr *prsrc)

Personally, it's a bit "confusing" (at best) to have the Format routine
be virStoragePRDefFormat

> +{
> +    xmlNodePtr child;
> +    virStoragePRDefPtr pr = NULL;
> +
> +    for (child = node->children; child; child = child->next) {
> +        if (child->type == XML_ELEMENT_NODE &&
> +            virXMLNodeNameEqual(child, "reservations")) {
> +
> +            if (!(pr = virStoragePRDefParseNode(node->doc, child)))
> +                return -1;
> +
> +            *prsrc = pr;
> +            return 0;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +
>  int
>  virDomainDiskSourceParse(xmlNodePtr node,
>                           xmlXPathContextPtr ctxt,
> @@ -8657,6 +8687,9 @@ virDomainDiskSourceParse(xmlNodePtr node,
>      if (virDomainDiskSourceEncryptionParse(node, &src->encryption) < 0)
>          goto cleanup;
>  
> +    if (virDomainDiskSourcePRParse(node, &src->pr) < 0)
> +        goto cleanup;
> +
>      if (virDomainDiskSourcePrivateDataParse(ctxt, src, flags, xmlopt) < 0)
>          goto cleanup;
>  
> @@ -22940,6 +22973,9 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf,
>              virStorageEncryptionFormat(&childBuf, src->encryption) < 0)
>              goto error;
>  
> +        if (src->pr)
> +            virStoragePRDefFormat(&childBuf, src->pr);
> +
>          if (virDomainDiskSourceFormatPrivateData(&childBuf, src, flags, xmlopt) < 0)
>              goto error;
>  
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 03f23dbab..071642c00 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2779,6 +2779,9 @@ virStorageNetHostDefFree;
>  virStorageNetHostTransportTypeFromString;
>  virStorageNetHostTransportTypeToString;
>  virStorageNetProtocolTypeToString;
> +virStoragePRDefFormat;
> +virStoragePRDefFree;
> +virStoragePRDefParseNode;
>  virStorageSourceBackingStoreClear;
>  virStorageSourceClear;
>  virStorageSourceCopy;
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index d1972d6d1..833d69f6d 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -1911,6 +1911,153 @@ virStorageAuthDefFormat(virBufferPtr buf,
>  }
>  
>  
> +void
> +virStoragePRDefFree(virStoragePRDefPtr prd)
> +{
> +    if (!prd)
> +        return;
> +
> +    VIR_FREE(prd->path);
> +    VIR_FREE(prd);
> +}
> +
> +
> +static virStoragePRDefPtr
> +virStoragePRDefParseXML(xmlXPathContextPtr ctxt)
> +{
> +    virStoragePRDefPtr prd, ret = NULL;
> +    char *enabled = NULL;
> +    char *managed = NULL;
> +    char *type = NULL;
> +    char *path = NULL;
> +    char *mode = NULL;
> +
> +    if (VIR_ALLOC(prd) < 0)
> +        return NULL;
> +
> +    if (!(enabled = virXPathString("string(./@enabled)", ctxt))) {
> +        virReportError(VIR_ERR_XML_ERROR, "%s",
> +                       _("missing @enabled attribute for <reservations/>"));
> +        goto cleanup;
> +    }
> +
> +    if ((prd->enabled = virTristateBoolTypeFromString(enabled)) <= 0) {
> +        virReportError(VIR_ERR_XML_ERROR,
> +                       _("invalid value for 'enabled': %s"), NULLSTR(enabled));
> +        goto cleanup;
> +    }
> +
> +    if (prd->enabled == VIR_TRISTATE_BOOL_YES) {
> +        managed = virXPathString("string(./@managed)", ctxt);
> +        if ((prd->managed = virTristateBoolTypeFromString(managed)) <= 0) {
> +            virReportError(VIR_ERR_XML_ERROR,
> +                           _("invalid value for 'managed': %s"), NULLSTR(managed));
> +            goto cleanup;
> +        }
> +
> +        if (prd->managed == VIR_TRISTATE_BOOL_NO) {

Consider my thoughts above vis-a-vis managed.

> +            type = virXPathString("string(./source[1]/@type)", ctxt);
> +            path = virXPathString("string(./source[1]/@path)", ctxt);
> +            mode = virXPathString("string(./source[1]/@mode)", ctxt);
> +
> +            if (!type) {
> +                virReportError(VIR_ERR_XML_ERROR, "%s",
> +                               _("missing connection type"));
> +                goto cleanup;
> +            }
> +
> +            if (!path) {
> +                virReportError(VIR_ERR_XML_ERROR, "%s",
> +                               _("missing path"));

missing connection path  (to be consistent)

> +                goto cleanup;
> +            }
> +
> +            if (!mode) {
> +                virReportError(VIR_ERR_XML_ERROR, "%s",
> +                               _("missing connection mode"));
> +                goto cleanup;
> +            }
> +
> +            if (STRNEQ(type, "unix")) {
> +                virReportError(VIR_ERR_XML_ERROR,
> +                               _("unsupported type: %s"), type);
> +                goto cleanup;
> +            }
> +
> +            if (STRNEQ(mode, "client")) {
> +                virReportError(VIR_ERR_XML_ERROR,
> +                               _("unsupported mode: %s"),  mode);
> +                goto cleanup;
> +            }

BTW: Seems pretty reasonable to add the same reconnect options that
vhostuser has. I mean if the helper process dies does libvirt need to be
restarted in order to recognize and restart? Or does it restart after
some time period when it recognizes the helper process died?

> +
> +            prd->path = path;
> +            path = NULL;
> +
> +        }
> +    }
> +
> +    ret = prd;
> +    prd = NULL;
> +
> + cleanup:
> +    VIR_FREE(mode);
> +    VIR_FREE(path);
> +    VIR_FREE(type);
> +    VIR_FREE(managed);
> +    VIR_FREE(enabled);
> +    virStoragePRDefFree(prd);
> +    return ret;
> +}
> +
> +
> +virStoragePRDefPtr
> +virStoragePRDefParseNode(xmlDocPtr xml,
> +                         xmlNodePtr root)
> +{
> +    xmlXPathContextPtr ctxt = NULL;
> +    virStoragePRDefPtr prd = NULL;
> +
> +    ctxt = xmlXPathNewContext(xml);
> +    if (ctxt == NULL) {
> +        virReportOOMError();
> +        goto cleanup;
> +    }
> +
> +    ctxt->node = root;
> +    prd = virStoragePRDefParseXML(ctxt);
> +
> + cleanup:
> +    xmlXPathFreeContext(ctxt);
> +    return prd;
> +}
> +
> +
> +void
> +virStoragePRDefFormat(virBufferPtr buf,
> +                      virStoragePRDefPtr prd)
> +{
> +    virBufferAsprintf(buf, "<reservations enabled='%s'",
> +                      virTristateBoolTypeToString(prd->enabled));

So if someone changes enabled from 'yes' to 'no', then if they had
anything in the <source> buffer it'd be lost. So I see no reason for enabled

> +    if (prd->enabled == VIR_TRISTATE_BOOL_YES) {
> +        virBufferAsprintf(buf, " managed='%s'",
> +                          virTristateBoolTypeToString(prd->managed));
> +        if (prd->managed == VIR_TRISTATE_BOOL_NO) {

As noted earlier this seems backwards to me.

> +            virBufferAddLit(buf, ">\n");
> +            virBufferAdjustIndent(buf, 2);
> +            virBufferAddLit(buf, "<source type='unix'");
> +            virBufferEscapeString(buf, " path='%s'", prd->path);
> +            virBufferAddLit(buf, " mode='client'/>\n");

So no chance of this being 'server', thus what's the point?

> +            virBufferAdjustIndent(buf, -2);
> +            virBufferAddLit(buf, "</reservations>\n");
> +        } else {
> +            virBufferAddLit(buf, "/>\n");
> +        }
> +    } else {
> +        virBufferAddLit(buf, "/>\n");
> +    }
> +}
> +
> +
>  virSecurityDeviceLabelDefPtr
>  virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src,
>                                      const char *model)
> @@ -2289,6 +2436,7 @@ virStorageSourceClear(virStorageSourcePtr def)
>      virBitmapFree(def->features);
>      VIR_FREE(def->compat);
>      virStorageEncryptionFree(def->encryption);
> +    virStoragePRDefFree(def->pr);
>      virStorageSourceSeclabelsClear(def);
>      virStoragePermsFree(def->perms);
>      VIR_FREE(def->timestamps);
> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
> index 0095cd138..968653660 100644
> --- a/src/util/virstoragefile.h
> +++ b/src/util/virstoragefile.h
> @@ -216,6 +216,14 @@ struct _virStorageAuthDef {
>      virSecretLookupTypeDef seclookupdef;
>  };
>  
> +typedef struct _virStoragePRDef virStoragePRDef;
> +typedef virStoragePRDef *virStoragePRDefPtr;
> +struct _virStoragePRDef {
> +    int enabled; /* enum virTristateBool */
> +    int managed; /* enum virTristateBool */
> +    char *path;
> +};
> +
>  typedef struct _virStorageDriverData virStorageDriverData;
>  typedef virStorageDriverData *virStorageDriverDataPtr;
>  
> @@ -243,6 +251,7 @@ struct _virStorageSource {
>      bool authInherited;
>      virStorageEncryptionPtr encryption;
>      bool encryptionInherited;
> +    virStoragePRDefPtr pr;
>  
>      virObjectPtr privateData;
>  
> @@ -369,6 +378,12 @@ virStorageAuthDefPtr virStorageAuthDefCopy(const virStorageAuthDef *src);
>  virStorageAuthDefPtr virStorageAuthDefParse(xmlDocPtr xml, xmlNodePtr root);
>  void virStorageAuthDefFormat(virBufferPtr buf, virStorageAuthDefPtr authdef);
>  
> +void virStoragePRDefFree(virStoragePRDefPtr prd);
> +virStoragePRDefPtr virStoragePRDefParseNode(xmlDocPtr xml,
> +                                            xmlNodePtr root);
> +void virStoragePRDefFormat(virBufferPtr buf,
> +                           virStoragePRDefPtr prd);
> +
>  virSecurityDeviceLabelDefPtr
>  virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src,
>                                      const char *model);
> diff --git a/tests/qemuxml2argvdata/disk-virtio-scsi-reservations-not-managed.xml b/tests/qemuxml2argvdata/disk-virtio-scsi-reservations-not-managed.xml
> new file mode 100644
> index 000000000..8ee623a70
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/disk-virtio-scsi-reservations-not-managed.xml
> @@ -0,0 +1,40 @@
> +<domain type='qemu'>
> +  <name>QEMUGuest1</name>
> +  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> +  <memory unit='KiB'>219136</memory>
> +  <currentMemory unit='KiB'>219136</currentMemory>
> +  <vcpu placement='static'>8</vcpu>
> +  <os>
> +    <type arch='i686' machine='pc'>hvm</type>
> +    <boot dev='hd'/>
> +  </os>
> +  <clock offset='utc'/>
> +  <on_poweroff>destroy</on_poweroff>
> +  <on_reboot>restart</on_reboot>
> +  <on_crash>destroy</on_crash>
> +  <devices>
> +    <emulator>/usr/bin/qemu-system-i686</emulator>
> +    <disk type='block' device='lun'>
> +      <source dev='/dev/HostVG/QEMUGuest1'>
> +        <reservations enabled='yes' managed='no'>

As noted earlier the managed attribute seems backwards.

> +          <source type='unix' path='/path/to/qemu-pr-helper.sock' mode='client'/>
> +        </reservations>
> +      </source>
> +      <target dev='sdb' bus='scsi'/>
> +      <address type='drive' controller='0' bus='0' target='0' unit='0'/>
> +    </disk>
> +    <controller type='usb' index='0'>
> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
> +    </controller>
> +    <controller type='scsi' index='0' model='virtio-scsi'>
> +      <driver queues='8'/>
> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
> +    </controller>
> +    <controller type='pci' index='0' model='pci-root'/>
> +    <input type='mouse' bus='ps2'/>
> +    <input type='keyboard' bus='ps2'/>
> +    <memballoon model='virtio'>
> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
> +    </memballoon>
> +  </devices>
> +</domain>
> diff --git a/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.xml b/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.xml
> new file mode 100644
> index 000000000..874446f7b
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.xml
> @@ -0,0 +1,38 @@
> +<domain type='qemu'>
> +  <name>QEMUGuest1</name>
> +  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> +  <memory unit='KiB'>219136</memory>
> +  <currentMemory unit='KiB'>219136</currentMemory>
> +  <vcpu placement='static'>8</vcpu>
> +  <os>
> +    <type arch='i686' machine='pc'>hvm</type>
> +    <boot dev='hd'/>
> +  </os>
> +  <clock offset='utc'/>
> +  <on_poweroff>destroy</on_poweroff>
> +  <on_reboot>restart</on_reboot>
> +  <on_crash>destroy</on_crash>
> +  <devices>
> +    <emulator>/usr/bin/qemu-system-i686</emulator>
> +    <disk type='block' device='lun'>
> +      <source dev='/dev/HostVG/QEMUGuest1'>
> +        <reservations enabled='yes' managed='yes'/>

As I noted earlier this seems backwards.

John

> +      </source>
> +      <target dev='sdb' bus='scsi'/>
> +      <address type='drive' controller='0' bus='0' target='0' unit='0'/>
> +    </disk>
> +    <controller type='usb' index='0'>
> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
> +    </controller>
> +    <controller type='scsi' index='0' model='virtio-scsi'>
> +      <driver queues='8'/>
> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
> +    </controller>
> +    <controller type='pci' index='0' model='pci-root'/>
> +    <input type='mouse' bus='ps2'/>
> +    <input type='keyboard' bus='ps2'/>
> +    <memballoon model='virtio'>
> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
> +    </memballoon>
> +  </devices>
> +</domain>
> diff --git a/tests/qemuxml2xmloutdata/disk-virtio-scsi-reservations-not-managed.xml b/tests/qemuxml2xmloutdata/disk-virtio-scsi-reservations-not-managed.xml
> new file mode 120000
> index 000000000..f96d62cb6
> --- /dev/null
> +++ b/tests/qemuxml2xmloutdata/disk-virtio-scsi-reservations-not-managed.xml
> @@ -0,0 +1 @@
> +../qemuxml2argvdata/disk-virtio-scsi-reservations-not-managed.xml
> \ No newline at end of file
> diff --git a/tests/qemuxml2xmloutdata/disk-virtio-scsi-reservations.xml b/tests/qemuxml2xmloutdata/disk-virtio-scsi-reservations.xml
> new file mode 120000
> index 000000000..dde52fd1d
> --- /dev/null
> +++ b/tests/qemuxml2xmloutdata/disk-virtio-scsi-reservations.xml
> @@ -0,0 +1 @@
> +../qemuxml2argvdata/disk-virtio-scsi-reservations.xml
> \ No newline at end of file
> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> index 0eb9e6c77..58cbf8438 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -531,6 +531,10 @@ mymain(void)
>              QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_VIRTIO_SCSI);
>      DO_TEST("disk-virtio-scsi-num_queues",
>              QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_VIRTIO_SCSI);
> +    DO_TEST("disk-virtio-scsi-reservations",
> +            QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_VIRTIO_SCSI);
> +    DO_TEST("disk-virtio-scsi-reservations-not-managed",
> +            QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_VIRTIO_SCSI);
>      DO_TEST("disk-virtio-scsi-cmd_per_lun",
>              QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_VIRTIO_SCSI);
>      DO_TEST("disk-virtio-scsi-max_sectors",
> 




More information about the libvir-list mailing list