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
> ---
> 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 @@
>
>
>
> -
> +
> mode='client'/>
>
>
> @@ -2952,17 +2952,16 @@
>Since libvirt 4.4.0, the
> reservations can be a sub-element of the
> source 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
> -enabled with accepted values yes and
> -no. If the feature is enabled, then there's another
> -mandatory attribute managed (accepted values are the
> -same as for enabled) 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 source, which currently accepts only the
> -following attributes: type with one value
> -unix, path with path the socket, and
> +managed with accepted values yes and
> +no. If managed 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 source,
s/in child/in the child/
> +which currently accepts only the following attributes:
> +type with one value unix,
> +path with path the socket, and
s/with path the socket/path to the socket/
> finally mode which accepts one value
> client 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
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
> ---
> 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 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
> ---
> 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
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
---
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 @@
-
+
@@ -2952,17 +2952,16 @@
Since libvirt 4.4.0, the
reservations can be a sub-element of the
source 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
-enabled with accepted values yes and
-no. If the feature is enabled, then there's another
-mandatory attribute managed (accepted values are the
-same as for enabled) 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 source, which currently accepts only the
-following attributes: type with one value
-unix, path with path the socket, and
+managed with accepted values yes and
+no. If managed 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 source,
+which currently accepts only the following attributes:
+type with one value unix,
+path with path the socket, and
finally mode which accepts one value
client 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 @@
-
-
-
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 "));
+ _("missing @managed attribute for "));
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
"));
+ _("missing connection
