Everything can be disabled by not using the parent element. There's no
need to store this explicitly. Additionally it does not add any value
since any configuration is dropped if enabled='no' is configured.
Drop the attribute and adjust the code accordingly.t
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
docs/formatdomain.html.in | 21 ++--
docs/schemas/storagecommon.rng | 3 -
src/util/virstoragefile.c | 117 +++++++++------------
src/util/virstoragefile.h | 1 -
.../disk-virtio-scsi-reservations.xml | 4 +-
5 files changed, 59 insertions(+), 87 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 80172c18d0..d69a669259 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -2583,7 +2583,7 @@
<disk type='block' device='lun'>
<driver name='qemu' type='raw'/>
<source dev='/dev/sda'>
- <reservations enabled='yes' managed='no'>
+ <reservations managed='no'>
<source type='unix' path='/path/to/qemu-pr-helper' mode='client'/>
</reservations>
<target dev='sda' bus='scsi'/>
@@ -2952,17 +2952,16 @@
<dd><span class="since">Since libvirt 4.4.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 enables persistent reservations for SCSI
+ If present it enables 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 a helper process. When the PR is unmanaged, then hypervisor
- acts as a client and path to server 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
+ <code>managed</code> with accepted values <code>yes</code> and
+ <code>no</code>. If <code>managed</code> is enabled libvirt prepares
+ and manages any resources needed for the feature. When the PR is
+ unmanaged, then hypervisor acts as a client and path to server
+ 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 one value
<code>client</code> and specifies the role of hypervisor.
It's recommended to allow libvirt manage the persistent
diff --git a/docs/schemas/storagecommon.rng b/docs/schemas/storagecommon.rng
index eed0b33347..cb4f14f52f 100644
--- a/docs/schemas/storagecommon.rng
+++ b/docs/schemas/storagecommon.rng
@@ -75,9 +75,6 @@
<define name='reservations'>
<element name='reservations'>
- <attribute name='enabled'>
- <ref name='virYesNo'/>
- </attribute>
<optional>
<attribute name='managed'>
<ref name='virYesNo'/>
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 87c3499561..d6907e47bb 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1906,8 +1906,8 @@ virStoragePRDefFree(virStoragePRDefPtr prd)
virStoragePRDefPtr
virStoragePRDefParseXML(xmlXPathContextPtr ctxt)
{
- virStoragePRDefPtr prd, ret = NULL;
- char *enabled = NULL;
+ virStoragePRDefPtr prd;
+ virStoragePRDefPtr ret = NULL;
char *managed = NULL;
char *type = NULL;
char *path = NULL;
@@ -1916,81 +1916,65 @@ virStoragePRDefParseXML(xmlXPathContextPtr ctxt)
if (VIR_ALLOC(prd) < 0)
return NULL;
- if (!(enabled = virXPathString("string(./@enabled)", ctxt))) {
+ if (!(managed = virXPathString("string(./@managed)", ctxt))) {
virReportError(VIR_ERR_XML_ERROR, "%s",
- _("missing @enabled attribute for <reservations/>"));
+ _("missing @managed attribute for <reservations/>"));
goto cleanup;
}
- if ((prd->enabled = virTristateBoolTypeFromString(enabled)) <= 0) {
+ if ((prd->managed = virTristateBoolTypeFromString(managed)) <= 0) {
virReportError(VIR_ERR_XML_ERROR,
- _("invalid value for 'enabled': %s"), enabled);
+ _("invalid value for 'managed': %s"), managed);
goto cleanup;
}
- if (prd->enabled == VIR_TRISTATE_BOOL_YES) {
- if (!(managed = virXPathString("string(./@managed)", ctxt))) {
+ if (prd->managed == VIR_TRISTATE_BOOL_NO) {
+ 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 @managed attribute for <reservations/>"));
+ _("missing connection type for <reservations/>"));
goto cleanup;
}
- if ((prd->managed = virTristateBoolTypeFromString(managed)) <= 0) {
- virReportError(VIR_ERR_XML_ERROR,
- _("invalid value for 'managed': %s"), managed);
+ if (!path) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("missing path for <reservations/>"));
goto cleanup;
}
- if (prd->managed == VIR_TRISTATE_BOOL_NO) {
- 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 for <reservations/>"));
- goto cleanup;
- }
-
- if (!path) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("missing path for <reservations/>"));
- goto cleanup;
- }
-
- if (!mode) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("missing connection mode for <reservations/>"));
- goto cleanup;
- }
-
- if (STRNEQ(type, "unix")) {
- virReportError(VIR_ERR_XML_ERROR,
- _("unsupported connection type for <reservations/>: %s"),
- type);
- goto cleanup;
- }
+ if (!mode) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("missing connection mode for <reservations/>"));
+ goto cleanup;
+ }
- if (STRNEQ(mode, "client")) {
- virReportError(VIR_ERR_XML_ERROR,
- _("unsupported connection mode for <reservations/>: %s"),
- mode);
- goto cleanup;
- }
+ if (STRNEQ(type, "unix")) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("unsupported connection type for <reservations/>: %s"),
+ type);
+ goto cleanup;
+ }
- VIR_STEAL_PTR(prd->path, path);
+ if (STRNEQ(mode, "client")) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("unsupported connection mode for <reservations/>: %s"),
+ mode);
+ goto cleanup;
}
+
+ VIR_STEAL_PTR(prd->path, path);
}
- ret = prd;
- prd = NULL;
+ VIR_STEAL_PTR(ret, prd);
cleanup:
VIR_FREE(mode);
VIR_FREE(path);
VIR_FREE(type);
VIR_FREE(managed);
- VIR_FREE(enabled);
virStoragePRDefFree(prd);
return ret;
}
@@ -2000,22 +1984,16 @@ void
virStoragePRDefFormat(virBufferPtr buf,
virStoragePRDefPtr prd)
{
- virBufferAsprintf(buf, "<reservations enabled='%s'",
- virTristateBoolTypeToString(prd->enabled));
- if (prd->enabled == VIR_TRISTATE_BOOL_YES) {
- virBufferAsprintf(buf, " managed='%s'",
- virTristateBoolTypeToString(prd->managed));
- if (prd->managed == VIR_TRISTATE_BOOL_NO) {
- virBufferAddLit(buf, ">\n");
- virBufferAdjustIndent(buf, 2);
- virBufferAddLit(buf, "<source type='unix'");
- virBufferEscapeString(buf, " path='%s'", prd->path);
- virBufferAddLit(buf, " mode='client'/>\n");
- virBufferAdjustIndent(buf, -2);
- virBufferAddLit(buf, "</reservations>\n");
- } else {
- virBufferAddLit(buf, "/>\n");
- }
+ virBufferAsprintf(buf, "<reservations managed='%s'",
+ virTristateBoolTypeToString(prd->managed));
+ if (prd->managed == VIR_TRISTATE_BOOL_NO) {
+ virBufferAddLit(buf, ">\n");
+ virBufferAdjustIndent(buf, 2);
+ virBufferAddLit(buf, "<source type='unix'");
+ virBufferEscapeString(buf, " path='%s'", prd->path);
+ virBufferAddLit(buf, " mode='client'/>\n");
+ virBufferAdjustIndent(buf, -2);
+ virBufferAddLit(buf, "</reservations>\n");
} else {
virBufferAddLit(buf, "/>\n");
}
@@ -2032,8 +2010,7 @@ virStoragePRDefIsEqual(virStoragePRDefPtr a,
if (!a || !b)
return false;
- if (a->enabled != b->enabled ||
- a->managed != b->managed ||
+ if (a->managed != b->managed ||
STRNEQ_NULLABLE(a->path, b->path))
return false;
@@ -2044,7 +2021,7 @@ virStoragePRDefIsEqual(virStoragePRDefPtr a,
bool
virStoragePRDefIsEnabled(virStoragePRDefPtr prd)
{
- return prd && prd->enabled == VIR_TRISTATE_BOOL_YES;
+ return !!prd;
}
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index 0bba016e4e..ec49152880 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -219,7 +219,6 @@ struct _virStorageAuthDef {
typedef struct _virStoragePRDef virStoragePRDef;
typedef virStoragePRDef *virStoragePRDefPtr;
struct _virStoragePRDef {
- int enabled; /* enum virTristateBool */
int managed; /* enum virTristateBool */
char *path;
};
diff --git a/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.xml b/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.xml
index 036c6e3c25..acad600ef8 100644
--- a/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.xml
+++ b/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.xml
@@ -17,7 +17,7 @@
<disk type='block' device='lun'>
<driver name='qemu' type='raw'/>
<source dev='/dev/HostVG/QEMUGuest1'>
- <reservations enabled='yes' managed='yes'/>
+ <reservations managed='yes'/>
</source>
<target dev='sda' bus='scsi'/>
<address type='drive' controller='0' bus='0' target='0' unit='0'/>
@@ -25,7 +25,7 @@
<disk type='block' device='lun'>
<driver name='qemu' type='raw'/>
<source dev='/dev/HostVG/QEMUGuest2'>
- <reservations enabled='yes' managed='no'>
+ <reservations managed='no'>
<source type='unix' path='/path/to/qemu-pr-helper.sock' mode='client'/>
</reservations>
</source>
--
2.16.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 05/14/2018 12:41 PM, Peter Krempa wrote: > Everything can be disabled by not using the parent element. There's no > need to store this explicitly. Additionally it does not add any value > since any configuration is dropped if enabled='no' is configured. > > Drop the attribute and adjust the code accordingly.t s/t$// > > Signed-off-by: Peter Krempa <pkrempa@redhat.com> > --- > docs/formatdomain.html.in | 21 ++-- > docs/schemas/storagecommon.rng | 3 - > src/util/virstoragefile.c | 117 +++++++++------------ > src/util/virstoragefile.h | 1 - > .../disk-virtio-scsi-reservations.xml | 4 +- > 5 files changed, 59 insertions(+), 87 deletions(-) Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 05/14/2018 06:41 AM, Peter Krempa wrote: > Everything can be disabled by not using the parent element. There's no > need to store this explicitly. Additionally it does not add any value > since any configuration is dropped if enabled='no' is configured. > > Drop the attribute and adjust the code accordingly.t > > Signed-off-by: Peter Krempa <pkrempa@redhat.com> > --- > docs/formatdomain.html.in | 21 ++-- > docs/schemas/storagecommon.rng | 3 - > src/util/virstoragefile.c | 117 +++++++++------------ > src/util/virstoragefile.h | 1 - > .../disk-virtio-scsi-reservations.xml | 4 +- > 5 files changed, 59 insertions(+), 87 deletions(-) > Hmm... I recall stating the same thing during v1 and v2 review, but got a deafening the design of XML was agreed upon already... John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 05/14/2018 06:41 AM, Peter Krempa wrote: > Everything can be disabled by not using the parent element. There's no > need to store this explicitly. Additionally it does not add any value > since any configuration is dropped if enabled='no' is configured. > > Drop the attribute and adjust the code accordingly.t > > Signed-off-by: Peter Krempa <pkrempa@redhat.com> > --- > docs/formatdomain.html.in | 21 ++-- > docs/schemas/storagecommon.rng | 3 - > src/util/virstoragefile.c | 117 +++++++++------------ > src/util/virstoragefile.h | 1 - > .../disk-virtio-scsi-reservations.xml | 4 +- > 5 files changed, 59 insertions(+), 87 deletions(-) > As I've worked may way forward - I reread the docs and needed to return here for commenting... > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index 80172c18d0..d69a669259 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -2583,7 +2583,7 @@ > <disk type='block' device='lun'> > <driver name='qemu' type='raw'/> > <source dev='/dev/sda'> > - <reservations enabled='yes' managed='no'> > + <reservations managed='no'> > <source type='unix' path='/path/to/qemu-pr-helper' mode='client'/> > </reservations> > <target dev='sda' bus='scsi'/> > @@ -2952,17 +2952,16 @@ > <dd><span class="since">Since libvirt 4.4.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 enables persistent reservations for SCSI > + If present it enables 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 a helper process. When the PR is unmanaged, then hypervisor > - acts as a client and path to server 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 > + <code>managed</code> with accepted values <code>yes</code> and > + <code>no</code>. If <code>managed</code> is enabled libvirt prepares > + and manages any resources needed for the feature. When the PR is s/for the feature././ s/PR is/persistent reservations are/ > + unmanaged, then hypervisor acts as a client and path to server s/then/then the/ s/and path to server/and the path to the server/ > + socket must be provided in child element <code>source</code>, s/in child/in the child/ > + which currently accepts only the following attributes: > + <code>type</code> with one value <code>unix</code>, > + <code>path</code> with path the socket, and s/with path the socket/path to the socket/ > finally <code>mode</code> which accepts one value > <code>client</code> and specifies the role of hypervisor. s/and specifies/specifying > It's recommended to allow libvirt manage the persistent John [...] -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.