5:41 a.m.
The XML design for the PR stuff is slightly weird so fix it and refactor
the code so that it will be much easier to use it with the blockdev
infrastructure.
Peter Krempa (13):
qemu: hotplug: Fix spacing around addition operator
qemu: alias: Allow passing alias of parent when generating PR manager
alias
qemu: command: Fix comment for qemuBuildPRManagerInfoProps
qemu: Move validation of PR manager support
util: storage: Drop pointless 'enabled' form PR definition
util: storage: Drop virStoragePRDefIsEnabled
util: storage: Allow passing <source> also for managed PR case
qemu: Assign managed PR path when preparing storage source
qemu: process: Change semantics of functions starting PR daemon
qemu: command: Move check whether PR manager object props need to be
built
conf: domain: Add helper to check whether a domain def requires use of
PR
util: storage: Store PR manager alias in the definition
qemu: hotplug: Replace qemuDomainDiskNeedRemovePR
docs/formatdomain.html.in | 21 ++--
docs/schemas/storagecommon.rng | 3 -
src/conf/domain_conf.c | 21 ++++
src/conf/domain_conf.h | 3 +
src/libvirt_private.syms | 2 +-
src/qemu/qemu_alias.c | 4 +-
src/qemu/qemu_alias.h | 2 +-
src/qemu/qemu_command.c | 61 +++-------
src/qemu/qemu_command.h | 6 +-
src/qemu/qemu_domain.c | 66 ++++++++---
src/qemu/qemu_domain.h | 3 +-
src/qemu/qemu_hotplug.c | 83 +++-----------
src/qemu/qemu_process.c | 40 ++-----
src/qemu/qemu_process.h | 5 +-
src/util/virstoragefile.c | 124 ++++++++-------------
src/util/virstoragefile.h | 5 +-
tests/qemustatusxml2xmldata/modern-in.xml | 4 +
.../disk-virtio-scsi-reservations.xml | 4 +-
tests/qemuxml2xmltest.c | 2 +-
19 files changed, 191 insertions(+), 268 deletions(-)
--
2.16.2
5:41 a.m.
New subject: [libvirt] [PATCH 01/13] qemu: hotplug: Fix spacing around addition operator
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index f3ff945787..1d748ccffb 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -484,7 +484,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
if (!(devstr = qemuBuildDriveDevStr(vm->def, disk, 0, priv->qemuCaps)))
goto error;
- if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0)
+ if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks + 1) < 0)
goto error;
qemuDomainObjEnterMonitor(driver, vm);
--
2.16.2
5:41 a.m.
New subject: [libvirt] [PATCH 02/13] qemu: alias: Allow passing alias of parent when generating PR manager alias
For use with blockdev the PR manager will be bound to a virStorageSource
rather than a virDomainDiskDef, so we will need to use the correct
alias.
Allow passing a string rather than the whole disk.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_alias.c | 4 ++--
src/qemu/qemu_alias.h | 2 +-
src/qemu/qemu_command.c | 4 ++--
src/qemu/qemu_hotplug.c | 2 +-
4 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
index bd714f7aee..578a33b284 100644
--- a/src/qemu/qemu_alias.c
+++ b/src/qemu/qemu_alias.c
@@ -783,11 +783,11 @@ qemuDomainGetManagedPRAlias(void)
char *
-qemuDomainGetUnmanagedPRAlias(const virDomainDiskDef *disk)
+qemuDomainGetUnmanagedPRAlias(const char *parentalias)
{
char *ret;
- ignore_value(virAsprintf(&ret, "pr-helper-%s", disk->info.alias));
+ ignore_value(virAsprintf(&ret, "pr-helper-%s", parentalias));
return ret;
}
diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h
index 76678658c0..51f64624d7 100644
--- a/src/qemu/qemu_alias.h
+++ b/src/qemu/qemu_alias.h
@@ -94,6 +94,6 @@ char *qemuAliasChardevFromDevAlias(const char *devAlias)
const char *qemuDomainGetManagedPRAlias(void);
-char *qemuDomainGetUnmanagedPRAlias(const virDomainDiskDef *disk);
+char *qemuDomainGetUnmanagedPRAlias(const char *parentalias);
#endif /* __QEMU_ALIAS_H__*/
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 11ad77f145..24b482efd8 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1482,7 +1482,7 @@ qemuBuildDriveSourcePR(virBufferPtr buf,
if (virStoragePRDefIsManaged(disk->src->pr))
defaultAlias = qemuDomainGetManagedPRAlias();
- else if (!(alias = qemuDomainGetUnmanagedPRAlias(disk)))
+ else if (!(alias = qemuDomainGetUnmanagedPRAlias(disk->info.alias)))
return -1;
@@ -9705,7 +9705,7 @@ qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
if (VIR_STRDUP(alias, qemuDomainGetManagedPRAlias()) < 0)
goto cleanup;
} else {
- if (!(alias = qemuDomainGetUnmanagedPRAlias(disk)))
+ if (!(alias = qemuDomainGetUnmanagedPRAlias(disk->info.alias)))
goto cleanup;
}
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 1d748ccffb..52e1abdcd3 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3842,7 +3842,7 @@ qemuDomainDiskNeedRemovePR(virDomainObjPtr vm,
return 0;
if (!virStoragePRDefIsManaged(disk->src->pr)) {
- *aliasret = qemuDomainGetUnmanagedPRAlias(disk);
+ *aliasret = qemuDomainGetUnmanagedPRAlias(disk->info.alias);
return *aliasret ? 0 : -1;
}
--
2.16.2
5:41 a.m.
New subject: [libvirt] [PATCH 03/13] qemu: command: Fix comment for qemuBuildPRManagerInfoProps
The comment did not accurately describe the arguments.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_command.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 24b482efd8..d84cf6dffc 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9667,8 +9667,10 @@ qemuBuildPanicCommandLine(virCommandPtr cmd,
/**
* qemuBuildPRManagerInfoProps:
- * @prd: disk PR runtime info
- * @propsret: JSON properties to return
+ * @vm: domain object
+ * @disk: disk definition
+ * @propsret: Returns JSON object containing properties of the pr-manager-helper object
+ * @aliasret: alias of the pr-manager-helper object
*
* Build the JSON properties for the pr-manager object.
*
--
2.16.2
5:41 a.m.
New subject: [libvirt] [PATCH 04/13] qemu: Move validation of PR manager support
Disk source definition should be validated in
qemuDomainValidateStorageSource rather than in individual generators of
command line arguments.
Change to the XML2XML test is required since now the definition is
actually validated at define time.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_command.c | 7 -------
src/qemu/qemu_domain.c | 7 +++++++
tests/qemuxml2xmltest.c | 2 +-
3 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index d84cf6dffc..29ca2005a0 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9683,7 +9683,6 @@ qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
virJSONValuePtr *propsret,
char **aliasret)
{
- qemuDomainObjPrivatePtr priv = vm->privateData;
char *socketPath = NULL;
char *alias = NULL;
int ret = -1;
@@ -9694,12 +9693,6 @@ qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
if (!virStoragePRDefIsEnabled(disk->src->pr))
return 0;
- if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_PR_MANAGER_HELPER)) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("reservations not supported with this QEMU binary"));
- return ret;
- }
-
if (!(socketPath = qemuDomainGetPRSocketPath(vm, disk->src->pr)))
return ret;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 233327b906..611a96d6be 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4204,6 +4204,13 @@ qemuDomainValidateStorageSource(virStorageSourcePtr src,
}
}
+ if (virStoragePRDefIsEnabled(src->pr) &&
+ !virQEMUCapsGet(qemuCaps, QEMU_CAPS_PR_MANAGER_HELPER)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("reservations not supported with this QEMU binary"));
+ return -1;
+ }
+
return 0;
}
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 762e0e2d25..44cba97d4a 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -388,7 +388,7 @@ mymain(void)
DO_TEST("disk-virtio-scsi-num_queues",
QEMU_CAPS_VIRTIO_SCSI);
DO_TEST("disk-virtio-scsi-reservations",
- QEMU_CAPS_VIRTIO_SCSI);
+ QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_PR_MANAGER_HELPER);
DO_TEST("disk-virtio-scsi-cmd_per_lun",
QEMU_CAPS_VIRTIO_SCSI);
DO_TEST("disk-virtio-scsi-max_sectors",
--
2.16.2
5:41 a.m.
New subject: [libvirt] [PATCH 05/13] util: storage: Drop pointless 'enabled' form PR definition
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(a)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
10:19 a.m.
New subject: [libvirt] [PATCH 05/13] util: storage: Drop pointless 'enabled' form PR definition
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(a)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
1:39 p.m.
New subject: [libvirt] [PATCH 05/13] util: storage: Drop pointless 'enabled' form PR definition
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(a)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
2:06 p.m.
New subject: [libvirt] [PATCH 05/13] util: storage: Drop pointless 'enabled' form PR definition
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(a)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
[...]
5:41 a.m.
New subject: [libvirt] [PATCH 06/13] util: storage: Drop virStoragePRDefIsEnabled
The function now does not do anything useful. Replace it by the pointer
check.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/libvirt_private.syms | 1 -
src/qemu/qemu_command.c | 4 ++--
src/qemu/qemu_domain.c | 8 ++++----
src/qemu/qemu_hotplug.c | 2 +-
src/util/virstoragefile.c | 7 -------
src/util/virstoragefile.h | 1 -
6 files changed, 7 insertions(+), 16 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index d28a751ebd..dd10be9753 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2804,7 +2804,6 @@ virStorageNetHostTransportTypeToString;
virStorageNetProtocolTypeToString;
virStoragePRDefFormat;
virStoragePRDefFree;
-virStoragePRDefIsEnabled;
virStoragePRDefIsEqual;
virStoragePRDefIsManaged;
virStoragePRDefParseXML;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 29ca2005a0..2bdba7734a 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1477,7 +1477,7 @@ qemuBuildDriveSourcePR(virBufferPtr buf,
char *alias = NULL;
const char *defaultAlias = NULL;
- if (!virStoragePRDefIsEnabled(disk->src->pr))
+ if (!disk->src->pr)
return 0;
if (virStoragePRDefIsManaged(disk->src->pr))
@@ -9690,7 +9690,7 @@ qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
*propsret = NULL;
*aliasret = NULL;
- if (!virStoragePRDefIsEnabled(disk->src->pr))
+ if (!disk->src->pr)
return 0;
if (!(socketPath = qemuDomainGetPRSocketPath(vm, disk->src->pr)))
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 611a96d6be..c8d2daa26f 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4204,7 +4204,7 @@ qemuDomainValidateStorageSource(virStorageSourcePtr src,
}
}
- if (virStoragePRDefIsEnabled(src->pr) &&
+ if (src->pr &&
!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PR_MANAGER_HELPER)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("reservations not supported with this QEMU binary"));
@@ -10240,7 +10240,7 @@ qemuDomainSetupDisk(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED,
}
/* qemu-pr-helper might require access to /dev/mapper/control. */
- if (virStoragePRDefIsEnabled(disk->src->pr) &&
+ if (disk->src->pr &&
qemuDomainCreateDevice(DEVICE_MAPPER_CONTROL_PATH, data, true) < 0)
goto cleanup;
@@ -11273,7 +11273,7 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm,
}
/* qemu-pr-helper might require access to /dev/mapper/control. */
- if (virStoragePRDefIsEnabled(src->pr) &&
+ if (src->pr &&
(VIR_STRDUP(dmPath, DEVICE_MAPPER_CONTROL_PATH) < 0 ||
VIR_APPEND_ELEMENT_COPY(paths, npaths, dmPath) < 0))
goto cleanup;
@@ -12050,7 +12050,7 @@ qemuDomainGetPRSocketPath(virDomainObjPtr vm,
const char *defaultAlias = NULL;
char *ret = NULL;
- if (!virStoragePRDefIsEnabled(pr))
+ if (!pr)
return NULL;
if (virStoragePRDefIsManaged(pr)) {
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 52e1abdcd3..39c457e607 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3838,7 +3838,7 @@ qemuDomainDiskNeedRemovePR(virDomainObjPtr vm,
*aliasret = NULL;
*stopDaemon = false;
- if (!virStoragePRDefIsEnabled(disk->src->pr))
+ if (!disk->src->pr)
return 0;
if (!virStoragePRDefIsManaged(disk->src->pr)) {
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index d6907e47bb..c89bdb9e49 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -2018,13 +2018,6 @@ virStoragePRDefIsEqual(virStoragePRDefPtr a,
}
-bool
-virStoragePRDefIsEnabled(virStoragePRDefPtr prd)
-{
- return !!prd;
-}
-
-
bool
virStoragePRDefIsManaged(virStoragePRDefPtr prd)
{
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index ec49152880..3a90c60fa5 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -396,7 +396,6 @@ void virStoragePRDefFormat(virBufferPtr buf,
virStoragePRDefPtr prd);
bool virStoragePRDefIsEqual(virStoragePRDefPtr a,
virStoragePRDefPtr b);
-bool virStoragePRDefIsEnabled(virStoragePRDefPtr prd);
bool virStoragePRDefIsManaged(virStoragePRDefPtr prd);
virSecurityDeviceLabelDefPtr
--
2.16.2
5:41 a.m.
New subject: [libvirt] [PATCH 07/13] util: storage: Allow passing <source> also for managed PR case
To allow storing status information in the XML move the validation that
the 'path' is not valid for managed PR daemon case into
qemuDomainValidateStorageSource and allow parsing of the data even in
case when managed='yes'.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_domain.c | 18 +++++++++++++-----
src/util/virstoragefile.c | 37 ++++++++++++++++++-------------------
2 files changed, 31 insertions(+), 24 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index c8d2daa26f..eaa796260c 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4204,11 +4204,19 @@ qemuDomainValidateStorageSource(virStorageSourcePtr src,
}
}
- if (src->pr &&
- !virQEMUCapsGet(qemuCaps, QEMU_CAPS_PR_MANAGER_HELPER)) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("reservations not supported with this QEMU binary"));
- return -1;
+ if (src->pr) {
+ if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PR_MANAGER_HELPER)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("reservations not supported with this QEMU
binary"));
+ return -1;
+ }
+
+ if (virStoragePRDefIsManaged(src->pr) && src->pr->path) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("'path' attribute should not be provided for
"
+ "managed reservations"));
+ return -1;
+ }
}
return 0;
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index c89bdb9e49..dbbe758f30 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1928,11 +1928,11 @@ virStoragePRDefParseXML(xmlXPathContextPtr ctxt)
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);
+ type = virXPathString("string(./source[1]/@type)", ctxt);
+ path = virXPathString("string(./source[1]/@path)", ctxt);
+ mode = virXPathString("string(./source[1]/@mode)", ctxt);
+ if (prd->managed == VIR_TRISTATE_BOOL_NO || type || path || mode) {
if (!type) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("missing connection type for
<reservations/>"));
@@ -1950,24 +1950,23 @@ virStoragePRDefParseXML(xmlXPathContextPtr ctxt)
_("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 (STRNEQ(mode, "client")) {
- virReportError(VIR_ERR_XML_ERROR,
- _("unsupported connection mode for <reservations/>:
%s"),
- mode);
- goto cleanup;
- }
+ if (type && STRNEQ(type, "unix")) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("unsupported connection type for <reservations/>:
%s"),
+ type);
+ goto cleanup;
+ }
- VIR_STEAL_PTR(prd->path, path);
+ if (mode && STRNEQ(mode, "client")) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("unsupported connection mode for <reservations/>:
%s"),
+ mode);
+ goto cleanup;
}
+ VIR_STEAL_PTR(prd->path, path);
VIR_STEAL_PTR(ret, prd);
cleanup:
@@ -1986,7 +1985,7 @@ virStoragePRDefFormat(virBufferPtr buf,
{
virBufferAsprintf(buf, "<reservations managed='%s'",
virTristateBoolTypeToString(prd->managed));
- if (prd->managed == VIR_TRISTATE_BOOL_NO) {
+ if (prd->path) {
virBufferAddLit(buf, ">\n");
virBufferAdjustIndent(buf, 2);
virBufferAddLit(buf, "<source type='unix'");
--
2.16.2
5:41 a.m.
New subject: [libvirt] [PATCH 08/13] qemu: Assign managed PR path when preparing storage source
Rather than always checking which path to use pre-assign it when
preparing storage source.
This reduces the need to pass 'vm' around too much. For later use the
path can be retrieved from the status XML.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_command.c | 18 +++++-------------
src/qemu/qemu_command.h | 3 +--
src/qemu/qemu_domain.c | 35 ++++++++++++++++++++++-------------
src/qemu/qemu_domain.h | 3 +--
src/qemu/qemu_hotplug.c | 2 +-
src/qemu/qemu_process.c | 2 +-
6 files changed, 31 insertions(+), 32 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 2bdba7734a..7df9979cb2 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9667,7 +9667,6 @@ qemuBuildPanicCommandLine(virCommandPtr cmd,
/**
* qemuBuildPRManagerInfoProps:
- * @vm: domain object
* @disk: disk definition
* @propsret: Returns JSON object containing properties of the pr-manager-helper object
* @aliasret: alias of the pr-manager-helper object
@@ -9678,12 +9677,10 @@ qemuBuildPanicCommandLine(virCommandPtr cmd,
* -1 on failure (with error message set).
*/
int
-qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
- const virDomainDiskDef *disk,
+qemuBuildPRManagerInfoProps(const virDomainDiskDef *disk,
virJSONValuePtr *propsret,
char **aliasret)
{
- char *socketPath = NULL;
char *alias = NULL;
int ret = -1;
@@ -9693,9 +9690,6 @@ qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
if (!disk->src->pr)
return 0;
- if (!(socketPath = qemuDomainGetPRSocketPath(vm, disk->src->pr)))
- return ret;
-
if (virStoragePRDefIsManaged(disk->src->pr)) {
if (VIR_STRDUP(alias, qemuDomainGetManagedPRAlias()) < 0)
goto cleanup;
@@ -9705,7 +9699,7 @@ qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
}
if (virJSONValueObjectCreate(propsret,
- "s:path", socketPath,
+ "s:path", disk->src->pr->path,
NULL) < 0)
goto cleanup;
@@ -9713,14 +9707,12 @@ qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
ret = 0;
cleanup:
VIR_FREE(alias);
- VIR_FREE(socketPath);
return ret;
}
static int
-qemuBuildMasterPRCommandLine(virDomainObjPtr vm,
- virCommandPtr cmd,
+qemuBuildMasterPRCommandLine(virCommandPtr cmd,
const virDomainDef *def)
{
size_t i;
@@ -9740,7 +9732,7 @@ qemuBuildMasterPRCommandLine(virDomainObjPtr vm,
managedAdded = true;
}
- if (qemuBuildPRManagerInfoProps(vm, disk, &props, &alias) < 0)
+ if (qemuBuildPRManagerInfoProps(disk, &props, &alias) < 0)
goto cleanup;
if (!props)
@@ -9934,7 +9926,7 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
if (qemuBuildMasterKeyCommandLine(cmd, priv) < 0)
goto error;
- if (qemuBuildMasterPRCommandLine(vm, cmd, def) < 0)
+ if (qemuBuildMasterPRCommandLine(cmd, def) < 0)
goto error;
if (enableFips)
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index da1fe679fe..621592cd79 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -55,8 +55,7 @@ virCommandPtr qemuBuildCommandLine(virQEMUDriverPtr driver,
int **nicindexes);
/* Generate the object properties for pr-manager */
-int qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
- const virDomainDiskDef *disk,
+int qemuBuildPRManagerInfoProps(const virDomainDiskDef *disk,
virJSONValuePtr *propsret,
char **alias);
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index eaa796260c..92217e66fe 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -11930,6 +11930,22 @@ qemuDomainPrepareDiskCachemode(virDomainDiskDefPtr disk)
}
+static int
+qemuDomainPrepareStorageSourcePR(virStorageSourcePtr src,
+ qemuDomainObjPrivatePtr priv)
+{
+ if (!src->pr)
+ return 0;
+
+ if (virStoragePRDefIsManaged(src->pr)) {
+ if (!(src->pr->path = qemuDomainGetManagedPRSocketPath(priv)))
+ return -1;
+ }
+
+ return 0;
+}
+
+
int
qemuDomainPrepareDiskSource(virDomainDiskDefPtr disk,
qemuDomainObjPrivatePtr priv,
@@ -11946,6 +11962,9 @@ qemuDomainPrepareDiskSource(virDomainDiskDefPtr disk,
if (qemuDomainPrepareDiskSourceChain(disk, NULL, cfg, priv->qemuCaps) < 0)
return -1;
+ if (qemuDomainPrepareStorageSourcePR(disk->src, priv) < 0)
+ return -1;
+
return 0;
}
@@ -12051,22 +12070,12 @@ qemuProcessEventFree(struct qemuProcessEvent *event)
char *
-qemuDomainGetPRSocketPath(virDomainObjPtr vm,
- virStoragePRDefPtr pr)
+qemuDomainGetManagedPRSocketPath(qemuDomainObjPrivatePtr priv)
{
- qemuDomainObjPrivatePtr priv = vm->privateData;
- const char *defaultAlias = NULL;
char *ret = NULL;
- if (!pr)
- return NULL;
-
- if (virStoragePRDefIsManaged(pr)) {
- defaultAlias = qemuDomainGetManagedPRAlias();
- ignore_value(virAsprintf(&ret, "%s/%s.sock", priv->libDir,
defaultAlias));
- } else {
- ignore_value(VIR_STRDUP(ret, pr->path));
- }
+ ignore_value(virAsprintf(&ret, "%s/%s.sock", priv->libDir,
+ qemuDomainGetManagedPRAlias()));
return ret;
}
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 09969f606a..40d1d095a3 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -1006,7 +1006,6 @@ qemuDomainDiskCachemodeFlags(int cachemode,
bool *direct,
bool *noflush);
-char * qemuDomainGetPRSocketPath(virDomainObjPtr vm,
- virStoragePRDefPtr pr);
+char * qemuDomainGetManagedPRSocketPath(qemuDomainObjPrivatePtr priv);
#endif /* __QEMU_DOMAIN_H__ */
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 39c457e607..77d37e5ef6 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -401,7 +401,7 @@ qemuMaybeBuildPRManagerInfoProps(virDomainObjPtr vm,
return 0;
}
- return qemuBuildPRManagerInfoProps(vm, disk, propsret, aliasret);
+ return qemuBuildPRManagerInfoProps(disk, propsret, aliasret);
}
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index a280784764..42b91b39ac 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2659,7 +2659,7 @@ qemuProcessStartPRDaemon(virDomainObjPtr vm,
if ((pidfd = virPidFileAcquirePath(pidfile, false, -1)) < 0)
goto cleanup;
- if (!(socketPath = qemuDomainGetPRSocketPath(vm, disk->src->pr)))
+ if (!(socketPath = qemuDomainGetManagedPRSocketPath(priv)))
goto cleanup;
/* Remove stale socket */
--
2.16.2
3:27 p.m.
New subject: [libvirt] [PATCH 08/13] qemu: Assign managed PR path when preparing storage source
On 05/14/2018 06:41 AM, Peter Krempa wrote:
Rather than always checking which path to use pre-assign it when
preparing storage source.
This reduces the need to pass 'vm' around too much. For later use the
path can be retrieved from the status XML.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_command.c | 18 +++++-------------
src/qemu/qemu_command.h | 3 +--
src/qemu/qemu_domain.c | 35 ++++++++++++++++++++++-------------
src/qemu/qemu_domain.h | 3 +--
src/qemu/qemu_hotplug.c | 2 +-
src/qemu/qemu_process.c | 2 +-
6 files changed, 31 insertions(+), 32 deletions(-)
Strange this patch got posted twice once w/ your own CC and once without.
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 2bdba7734a..7df9979cb2 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9667,7 +9667,6 @@ qemuBuildPanicCommandLine(virCommandPtr cmd,
/**
* qemuBuildPRManagerInfoProps:
- * @vm: domain object
* @disk: disk definition
* @propsret: Returns JSON object containing properties of the pr-manager-helper object
* @aliasret: alias of the pr-manager-helper object
@@ -9678,12 +9677,10 @@ qemuBuildPanicCommandLine(virCommandPtr cmd,
* -1 on failure (with error message set).
*/
int
-qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
- const virDomainDiskDef *disk,
+qemuBuildPRManagerInfoProps(const virDomainDiskDef *disk,
virJSONValuePtr *propsret,
char **aliasret)
{
- char *socketPath = NULL;
char *alias = NULL;
int ret = -1;
@@ -9693,9 +9690,6 @@ qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
if (!disk->src->pr)
return 0;
- if (!(socketPath = qemuDomainGetPRSocketPath(vm, disk->src->pr)))
- return ret;
-
if (virStoragePRDefIsManaged(disk->src->pr)) {
if (VIR_STRDUP(alias, qemuDomainGetManagedPRAlias()) < 0)
goto cleanup;
@@ -9705,7 +9699,7 @@ qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
}
if (virJSONValueObjectCreate(propsret,
- "s:path", socketPath,
+ "s:path", disk->src->pr->path,
NULL) < 0)
goto cleanup;
@@ -9713,14 +9707,12 @@ qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
ret = 0;
cleanup:
VIR_FREE(alias);
- VIR_FREE(socketPath);
return ret;
}
static int
-qemuBuildMasterPRCommandLine(virDomainObjPtr vm,
- virCommandPtr cmd,
+qemuBuildMasterPRCommandLine(virCommandPtr cmd,
const virDomainDef *def)
{
size_t i;
@@ -9740,7 +9732,7 @@ qemuBuildMasterPRCommandLine(virDomainObjPtr vm,
managedAdded = true;
}
- if (qemuBuildPRManagerInfoProps(vm, disk, &props, &alias) < 0)
+ if (qemuBuildPRManagerInfoProps(disk, &props, &alias) < 0)
goto cleanup;
if (!props)
@@ -9934,7 +9926,7 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
if (qemuBuildMasterKeyCommandLine(cmd, priv) < 0)
goto error;
- if (qemuBuildMasterPRCommandLine(vm, cmd, def) < 0)
+ if (qemuBuildMasterPRCommandLine(cmd, def) < 0)
Rather than @vm - what was really desired is/was @priv, which is already
passed for qemuBuildMasterKeyCommandLine and could be for this too...
Thus not requiring the hunks that change path in disk->src->pr->path.
It is a static path beyond the priv->libDir part.
goto error;
if (enableFips)
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index da1fe679fe..621592cd79 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -55,8 +55,7 @@ virCommandPtr qemuBuildCommandLine(virQEMUDriverPtr driver,
int **nicindexes);
/* Generate the object properties for pr-manager */
-int qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
- const virDomainDiskDef *disk,
+int qemuBuildPRManagerInfoProps(const virDomainDiskDef *disk,
virJSONValuePtr *propsret,
char **alias);
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index eaa796260c..92217e66fe 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -11930,6 +11930,22 @@ qemuDomainPrepareDiskCachemode(virDomainDiskDefPtr disk)
}
+static int
+qemuDomainPrepareStorageSourcePR(virStorageSourcePtr src,
+ qemuDomainObjPrivatePtr priv)
+{
+ if (!src->pr)
+ return 0;
+
+ if (virStoragePRDefIsManaged(src->pr)) {
+ if (!(src->pr->path = qemuDomainGetManagedPRSocketPath(priv)))
+ return -1;
+ }
While I understand the eventual goal - assigning a path here would seem
to be "contrary" to qemuDomainValidateStorageSource in the previous
patch. IOW: ->path should not be provided for managed reservations.
Doesn't that mean from this point forward we need to be careful to not
save the resulting path for the persistent XML? Or am I lost in the
weeds again?
If this is to stay, then is this perhaps where :
<reservations managed='yes'>
<source type='unix' path='/somepath/ux.sck'
mode='client'/>
</reservations>
from patch 12 should be included for
tests/qemustatusxml2xmldata/modern-in.xml ?
John
+
+ return 0;
+}
+
+
int
qemuDomainPrepareDiskSource(virDomainDiskDefPtr disk,
qemuDomainObjPrivatePtr priv,
@@ -11946,6 +11962,9 @@ qemuDomainPrepareDiskSource(virDomainDiskDefPtr disk,
if (qemuDomainPrepareDiskSourceChain(disk, NULL, cfg, priv->qemuCaps) < 0)
return -1;
+ if (qemuDomainPrepareStorageSourcePR(disk->src, priv) < 0)
+ return -1;
+
return 0;
}
@@ -12051,22 +12070,12 @@ qemuProcessEventFree(struct qemuProcessEvent *event)
char *
-qemuDomainGetPRSocketPath(virDomainObjPtr vm,
- virStoragePRDefPtr pr)
+qemuDomainGetManagedPRSocketPath(qemuDomainObjPrivatePtr priv)
{
- qemuDomainObjPrivatePtr priv = vm->privateData;
- const char *defaultAlias = NULL;
char *ret = NULL;
- if (!pr)
- return NULL;
-
- if (virStoragePRDefIsManaged(pr)) {
- defaultAlias = qemuDomainGetManagedPRAlias();
- ignore_value(virAsprintf(&ret, "%s/%s.sock", priv->libDir,
defaultAlias));
- } else {
- ignore_value(VIR_STRDUP(ret, pr->path));
- }
+ ignore_value(virAsprintf(&ret, "%s/%s.sock", priv->libDir,
+ qemuDomainGetManagedPRAlias()));
return ret;
}
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 09969f606a..40d1d095a3 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -1006,7 +1006,6 @@ qemuDomainDiskCachemodeFlags(int cachemode,
bool *direct,
bool *noflush);
-char * qemuDomainGetPRSocketPath(virDomainObjPtr vm,
- virStoragePRDefPtr pr);
+char * qemuDomainGetManagedPRSocketPath(qemuDomainObjPrivatePtr priv);
#endif /* __QEMU_DOMAIN_H__ */
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 39c457e607..77d37e5ef6 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -401,7 +401,7 @@ qemuMaybeBuildPRManagerInfoProps(virDomainObjPtr vm,
return 0;
}
- return qemuBuildPRManagerInfoProps(vm, disk, propsret, aliasret);
+ return qemuBuildPRManagerInfoProps(disk, propsret, aliasret);
}
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index a280784764..42b91b39ac 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2659,7 +2659,7 @@ qemuProcessStartPRDaemon(virDomainObjPtr vm,
if ((pidfd = virPidFileAcquirePath(pidfile, false, -1)) < 0)
goto cleanup;
- if (!(socketPath = qemuDomainGetPRSocketPath(vm, disk->src->pr)))
+ if (!(socketPath = qemuDomainGetManagedPRSocketPath(priv)))
goto cleanup;
/* Remove stale socket */
10:44 a.m.
New subject: [libvirt] [PATCH 08/13] qemu: Assign managed PR path when preparing storage source
On Mon, May 14, 2018 at 16:27:17 -0400, John Ferlan wrote:
On 05/14/2018 06:41 AM, Peter Krempa wrote:
> Rather than always checking which path to use pre-assign it when
> preparing storage source.
>
> This reduces the need to pass 'vm' around too much. For later use the
> path can be retrieved from the status XML.
>
> Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
> ---
> src/qemu/qemu_command.c | 18 +++++-------------
> src/qemu/qemu_command.h | 3 +--
> src/qemu/qemu_domain.c | 35 ++++++++++++++++++++++-------------
> src/qemu/qemu_domain.h | 3 +--
> src/qemu/qemu_hotplug.c | 2 +-
> src/qemu/qemu_process.c | 2 +-
> 6 files changed, 31 insertions(+), 32 deletions(-)
>
Strange this patch got posted twice once w/ your own CC and once without.
Yes, my mailserver rejected sending of the next patch so I've retried it
manually without the CC and messed up selecting which ones I should
resend.
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 2bdba7734a..7df9979cb2 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -9667,7 +9667,6 @@ qemuBuildPanicCommandLine(virCommandPtr cmd,
>
> /**
> * qemuBuildPRManagerInfoProps:
> - * @vm: domain object
> * @disk: disk definition
> * @propsret: Returns JSON object containing properties of the pr-manager-helper
object
> * @aliasret: alias of the pr-manager-helper object
> @@ -9678,12 +9677,10 @@ qemuBuildPanicCommandLine(virCommandPtr cmd,
> * -1 on failure (with error message set).
> */
> int
> -qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
> - const virDomainDiskDef *disk,
> +qemuBuildPRManagerInfoProps(const virDomainDiskDef *disk,
> virJSONValuePtr *propsret,
> char **aliasret)
> {
> - char *socketPath = NULL;
> char *alias = NULL;
> int ret = -1;
>
> @@ -9693,9 +9690,6 @@ qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
> if (!disk->src->pr)
> return 0;
>
> - if (!(socketPath = qemuDomainGetPRSocketPath(vm, disk->src->pr)))
> - return ret;
> -
> if (virStoragePRDefIsManaged(disk->src->pr)) {
> if (VIR_STRDUP(alias, qemuDomainGetManagedPRAlias()) < 0)
> goto cleanup;
> @@ -9705,7 +9699,7 @@ qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
> }
>
> if (virJSONValueObjectCreate(propsret,
> - "s:path", socketPath,
> + "s:path", disk->src->pr->path,
> NULL) < 0)
> goto cleanup;
>
> @@ -9713,14 +9707,12 @@ qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
> ret = 0;
> cleanup:
> VIR_FREE(alias);
> - VIR_FREE(socketPath);
> return ret;
> }
>
>
> static int
> -qemuBuildMasterPRCommandLine(virDomainObjPtr vm,
> - virCommandPtr cmd,
> +qemuBuildMasterPRCommandLine(virCommandPtr cmd,
> const virDomainDef *def)
> {
> size_t i;
> @@ -9740,7 +9732,7 @@ qemuBuildMasterPRCommandLine(virDomainObjPtr vm,
> managedAdded = true;
> }
>
> - if (qemuBuildPRManagerInfoProps(vm, disk, &props, &alias) < 0)
> + if (qemuBuildPRManagerInfoProps(disk, &props, &alias) < 0)
> goto cleanup;
>
> if (!props)
> @@ -9934,7 +9926,7 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
> if (qemuBuildMasterKeyCommandLine(cmd, priv) < 0)
> goto error;
>
> - if (qemuBuildMasterPRCommandLine(vm, cmd, def) < 0)
> + if (qemuBuildMasterPRCommandLine(cmd, def) < 0)
Rather than @vm - what was really desired is/was @priv, which is already
passed for qemuBuildMasterKeyCommandLine and could be for this too...
Thus not requiring the hunks that change path in disk->src->pr->path.
It is a static path beyond the priv->libDir part.
> goto error;
>
> if (enableFips)
> diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
> index da1fe679fe..621592cd79 100644
> --- a/src/qemu/qemu_command.h
> +++ b/src/qemu/qemu_command.h
> @@ -55,8 +55,7 @@ virCommandPtr qemuBuildCommandLine(virQEMUDriverPtr driver,
> int **nicindexes);
>
> /* Generate the object properties for pr-manager */
> -int qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
> - const virDomainDiskDef *disk,
> +int qemuBuildPRManagerInfoProps(const virDomainDiskDef *disk,
> virJSONValuePtr *propsret,
> char **alias);
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index eaa796260c..92217e66fe 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -11930,6 +11930,22 @@ qemuDomainPrepareDiskCachemode(virDomainDiskDefPtr disk)
> }
>
>
> +static int
> +qemuDomainPrepareStorageSourcePR(virStorageSourcePtr src,
> + qemuDomainObjPrivatePtr priv)
> +{
> + if (!src->pr)
> + return 0;
> +
> + if (virStoragePRDefIsManaged(src->pr)) {
> + if (!(src->pr->path = qemuDomainGetManagedPRSocketPath(priv)))
> + return -1;
> + }
While I understand the eventual goal - assigning a path here would seem
to be "contrary" to qemuDomainValidateStorageSource in the previous
patch. IOW: ->path should not be provided for managed reservations.
Doesn't that mean from this point forward we need to be careful to not
save the resulting path for the persistent XML? Or am I lost in the
weeds again?
It should never be saved in persistent XML since the parser+validator
will not allow parsing it. Since we don't modify the persistent XML in
this case it's not a problem.
The validation code is not called for the status XML so that should be
covered as well.
If this is to stay, then is this perhaps where :
<reservations managed='yes'>
<source type='unix' path='/somepath/ux.sck'
mode='client'/>
</reservations>
from patch 12 should be included for
tests/qemustatusxml2xmldata/modern-in.xml ?
Hmm, yeah we can add that too.
5:45 a.m.
New subject: [libvirt] [PATCH 09/13] qemu: process: Change semantics of functions starting PR daemon
Libvirt only manages one PR daemon. This means that we don't need to
pass the 'disk' object and also rename the functions dealing with this
so that it's obvious we only deal with the managed PR daemon.
Signed-off-by: Peter Krempa <pkrempa@redhat st.com>
---
src/qemu/qemu_hotplug.c | 6 +++---
src/qemu/qemu_process.c | 37 ++++++++++++++++---------------------
src/qemu/qemu_process.h | 5 ++---
3 files changed, 21 insertions(+), 27 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 77d37e5ef6..3a26876c10 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -377,7 +377,7 @@ qemuDomainMaybeStartPRDaemon(virDomainObjPtr vm,
/* @disk requires qemu-pr-helper but none is running.
* Start it now. */
- if (qemuProcessStartPRDaemon(vm, disk) < 0)
+ if (qemuProcessStartManagedPRDaemon(vm) < 0)
return -1;
return 1;
@@ -567,7 +567,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
qemuDomainDelDiskSrcTLSObject(driver, vm, disk->src);
ignore_value(qemuHotplugPrepareDiskAccess(driver, vm, disk, NULL, true));
if (prdStarted)
- qemuProcessKillPRDaemon(vm);
+ qemuProcessKillManagedPRDaemon(vm);
goto cleanup;
}
@@ -3963,7 +3963,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
}
if (stopPRDaemon)
- qemuProcessKillPRDaemon(vm);
+ qemuProcessKillManagedPRDaemon(vm);
qemuDomainReleaseDeviceAddress(vm, &disk->info, src);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 42b91b39ac..af29bcc59e 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2566,7 +2566,7 @@ qemuProcessBuildPRHelperPidfilePath(virDomainObjPtr vm)
void
-qemuProcessKillPRDaemon(virDomainObjPtr vm)
+qemuProcessKillManagedPRDaemon(virDomainObjPtr vm)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
virErrorPtr orig_err;
@@ -2624,8 +2624,7 @@ qemuProcessStartPRDaemonHook(void *opaque)
int
-qemuProcessStartPRDaemon(virDomainObjPtr vm,
- const virDomainDiskDef *disk)
+qemuProcessStartManagedPRDaemon(virDomainObjPtr vm)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
virQEMUDriverPtr driver = priv->driver;
@@ -2640,10 +2639,6 @@ qemuProcessStartPRDaemon(virDomainObjPtr vm,
const unsigned long long timeout = 500000; /* ms */
int ret = -1;
- if (!virStoragePRDefIsManaged(disk->src->pr) ||
- priv->prDaemonRunning)
- return 0;
-
cfg = virQEMUDriverGetConfig(driver);
if (!virFileIsExecutable(cfg->prHelperName)) {
@@ -2734,7 +2729,7 @@ qemuProcessStartPRDaemon(virDomainObjPtr vm,
goto cleanup;
priv->prDaemonRunning = true;
- ret = 1;
+ ret = 0;
cleanup:
if (ret < 0) {
virCommandAbort(cmd);
@@ -2754,22 +2749,22 @@ qemuProcessStartPRDaemon(virDomainObjPtr vm,
static int
-qemuProcessMaybeStartPRDaemon(virDomainObjPtr vm)
+qemuProcessMaybeStartManagedPRDaemon(virDomainObjPtr vm)
{
+ bool hasManaged = false;
size_t i;
- int rv;
for (i = 0; i < vm->def->ndisks; i++) {
- const virDomainDiskDef *disk = vm->def->disks[i];
-
- if ((rv = qemuProcessStartPRDaemon(vm, disk)) < 0)
- return -1;
-
- if (rv > 0)
- return 1;
+ if (virStoragePRDefIsManaged(vm->def->disks[i]->src->pr)) {
+ hasManaged = true;
+ break;
+ }
}
- return 0;
+ if (!hasManaged)
+ return 0;
+
+ return qemuProcessStartManagedPRDaemon(vm);
}
@@ -6289,8 +6284,8 @@ qemuProcessLaunch(virConnectPtr conn,
if (qemuProcessResctrlCreate(driver, vm) < 0)
goto cleanup;
- VIR_DEBUG("Setting up PR daemon");
- if (qemuProcessMaybeStartPRDaemon(vm) < 0)
+ VIR_DEBUG("Setting up managed PR daemon");
+ if (qemuProcessMaybeStartManagedPRDaemon(vm) < 0)
goto cleanup;
VIR_DEBUG("Setting domain security labels");
@@ -6821,7 +6816,7 @@ void qemuProcessStop(virQEMUDriverPtr driver,
qemuDomainMasterKeyRemove(priv);
/* Do this before we delete the tree and remove pidfile. */
- qemuProcessKillPRDaemon(vm);
+ qemuProcessKillManagedPRDaemon(vm);
virFileDeleteTree(priv->libDir);
virFileDeleteTree(priv->channelTargetDir);
diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
index eda6695415..a0e34b1c85 100644
--- a/src/qemu/qemu_process.h
+++ b/src/qemu/qemu_process.h
@@ -205,9 +205,8 @@ int qemuProcessRefreshDisks(virQEMUDriverPtr driver,
virDomainObjPtr vm,
qemuDomainAsyncJob asyncJob);
-int qemuProcessStartPRDaemon(virDomainObjPtr vm,
- const virDomainDiskDef *disk);
+int qemuProcessStartManagedPRDaemon(virDomainObjPtr vm);
-void qemuProcessKillPRDaemon(virDomainObjPtr vm);
+void qemuProcessKillManagedPRDaemon(virDomainObjPtr vm);
#endif /* __QEMU_PROCESS_H__ */
--
2.16.2
3:33 p.m.
New subject: [libvirt] [PATCH 09/13] qemu: process: Change semantics of functions starting PR daemon
On 05/14/2018 06:45 AM, Peter Krempa wrote:
Libvirt only manages one PR daemon. This means that we don't need
to
pass the 'disk' object and also rename the functions dealing with this
so that it's obvious we only deal with the managed PR daemon.
Signed-off-by: Peter Krempa <pkrempa@redhat st.com>
^^^^
Something strange happened here.
---
src/qemu/qemu_hotplug.c | 6 +++---
src/qemu/qemu_process.c | 37 ++++++++++++++++---------------------
src/qemu/qemu_process.h | 5 ++---
3 files changed, 21 insertions(+), 27 deletions(-)
[...]
int
-qemuProcessStartPRDaemon(virDomainObjPtr vm,
- const virDomainDiskDef *disk)
+qemuProcessStartManagedPRDaemon(virDomainObjPtr vm)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
virQEMUDriverPtr driver = priv->driver;
@@ -2640,10 +2639,6 @@ qemuProcessStartPRDaemon(virDomainObjPtr vm,
const unsigned long long timeout = 500000; /* ms */
int ret = -1;
- if (!virStoragePRDefIsManaged(disk->src->pr) ||
- priv->prDaemonRunning)
- return 0;
-
cfg = virQEMUDriverGetConfig(driver);
if (!virFileIsExecutable(cfg->prHelperName)) {
@@ -2734,7 +2729,7 @@ qemuProcessStartPRDaemon(virDomainObjPtr vm,
goto cleanup;
priv->prDaemonRunning = true;
- ret = 1;
+ ret = 0;
Unrelated, but since callers only care about < 0, no big deal... I'm
assuming this is a holder from some other path which used the function
for a return value (e.g. like qemuDomainMaybeStartPRDaemon) at some
point during development.
No big deal - I don't expect this to be extracted, but was just paying
attention ;-)
John
cleanup:
if (ret < 0) {
virCommandAbort(cmd);
@@ -2754,22 +2749,22 @@ qemuProcessStartPRDaemon(virDomainObjPtr vm,
[...]
10:50 a.m.
New subject: [libvirt] [PATCH 09/13] qemu: process: Change semantics of functions starting PR daemon
On Mon, May 14, 2018 at 16:33:58 -0400, John Ferlan wrote:
On 05/14/2018 06:45 AM, Peter Krempa wrote:
> Libvirt only manages one PR daemon. This means that we don't need to
> pass the 'disk' object and also rename the functions dealing with this
> so that it's obvious we only deal with the managed PR daemon.
>
> Signed-off-by: Peter Krempa <pkrempa@redhat st.com>
^^^^
Something strange happened here.
Yeah, I've messed up while editing the commit message somehow. It also
broke sending of the patches later on.
> ---
> src/qemu/qemu_hotplug.c | 6 +++---
> src/qemu/qemu_process.c | 37 ++++++++++++++++---------------------
> src/qemu/qemu_process.h | 5 ++---
> 3 files changed, 21 insertions(+), 27 deletions(-)
>
[...]
> int
> -qemuProcessStartPRDaemon(virDomainObjPtr vm,
> - const virDomainDiskDef *disk)
> +qemuProcessStartManagedPRDaemon(virDomainObjPtr vm)
> {
> qemuDomainObjPrivatePtr priv = vm->privateData;
> virQEMUDriverPtr driver = priv->driver;
> @@ -2640,10 +2639,6 @@ qemuProcessStartPRDaemon(virDomainObjPtr vm,
> const unsigned long long timeout = 500000; /* ms */
> int ret = -1;
>
> - if (!virStoragePRDefIsManaged(disk->src->pr) ||
> - priv->prDaemonRunning)
> - return 0;
> -
> cfg = virQEMUDriverGetConfig(driver);
>
> if (!virFileIsExecutable(cfg->prHelperName)) {
> @@ -2734,7 +2729,7 @@ qemuProcessStartPRDaemon(virDomainObjPtr vm,
> goto cleanup;
>
> priv->prDaemonRunning = true;
> - ret = 1;
> + ret = 0;
Unrelated, but since callers only care about < 0, no big deal... I'm
assuming this is a holder from some other path which used the function
for a return value (e.g. like qemuDomainMaybeStartPRDaemon) at some
point during development.
No big deal - I don't expect this to be extracted, but was just paying
attention ;-)
The code path was actually exactly used in the case when
qemuDomainMaybeStartPRDaemon called into this function. Since the logic
determining whether it was started was extracted into that function (in
trimmed context) it now returns the correct thing there which is
expected and qemuProcessStartManagedPRDaemon does not have to do this
any more.
5:45 a.m.
New subject: [libvirt] [PATCH 10/13] qemu: command: Move check whether PR manager object props need to be built
Move it out of the formatter function and let the caller decide this.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_command.c | 9 +++------
src/qemu/qemu_hotplug.c | 3 +++
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 7df9979cb2..c38dde5a60 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9687,9 +9687,6 @@ qemuBuildPRManagerInfoProps(const virDomainDiskDef *disk,
*propsret = NULL;
*aliasret = NULL;
- if (!disk->src->pr)
- return 0;
-
if (virStoragePRDefIsManaged(disk->src->pr)) {
if (VIR_STRDUP(alias, qemuDomainGetManagedPRAlias()) < 0)
goto cleanup;
@@ -9725,6 +9722,9 @@ qemuBuildMasterPRCommandLine(virCommandPtr cmd,
for (i = 0; i < def->ndisks; i++) {
const virDomainDiskDef *disk = def->disks[i];
+ if (!disk->src->pr)
+ continue;
+
if (virStoragePRDefIsManaged(disk->src->pr)) {
if (managedAdded)
continue;
@@ -9735,9 +9735,6 @@ qemuBuildMasterPRCommandLine(virCommandPtr cmd,
if (qemuBuildPRManagerInfoProps(disk, &props, &alias) < 0)
goto cleanup;
- if (!props)
- continue;
-
if (!(tmp = virQEMUBuildObjectCommandlineFromJSON("pr-manager-helper",
alias,
props)))
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 3a26876c10..6557711ec1 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -395,6 +395,9 @@ qemuMaybeBuildPRManagerInfoProps(virDomainObjPtr vm,
*propsret = NULL;
*aliasret = NULL;
+ if (!disk->src->pr)
+ return 0;
+
if (virStoragePRDefIsManaged(disk->src->pr) &&
priv->prDaemonRunning) {
/* @disk requires qemu-pr-helper but there's already one running. */
--
2.16.2
3:46 p.m.
New subject: [libvirt] [PATCH 10/13] qemu: command: Move check whether PR manager object props need to be built
On 05/14/2018 06:45 AM, Peter Krempa wrote:
Move it out of the formatter function and let the caller decide this.
s/formatter/format/
I cannot recall our current consistency... Some times we seem to have
the lowest routine make the singular check and other times we have the
caller make the check.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_command.c | 9 +++------
src/qemu/qemu_hotplug.c | 3 +++
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 7df9979cb2..c38dde5a60 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9687,9 +9687,6 @@ qemuBuildPRManagerInfoProps(const virDomainDiskDef *disk,
*propsret = NULL;
*aliasret = NULL;
- if (!disk->src->pr)
- return 0;
-
if (virStoragePRDefIsManaged(disk->src->pr)) {
if (VIR_STRDUP(alias, qemuDomainGetManagedPRAlias()) < 0)
goto cleanup;
@@ -9725,6 +9722,9 @@ qemuBuildMasterPRCommandLine(virCommandPtr cmd,
for (i = 0; i < def->ndisks; i++) {
const virDomainDiskDef *disk = def->disks[i];
+ if (!disk->src->pr)
+ continue;
+
if (virStoragePRDefIsManaged(disk->src->pr)) {
if (managedAdded)
continue;
@@ -9735,9 +9735,6 @@ qemuBuildMasterPRCommandLine(virCommandPtr cmd,
if (qemuBuildPRManagerInfoProps(disk, &props, &alias) < 0)
goto cleanup;
- if (!props)
- continue;
-
This one seems unrelated... although at this point I'm not sure how it
could happen... I'd say keep it as is unless Michal had some other
reason or maybe remembers what it was there for originally.
John
if (!(tmp =
virQEMUBuildObjectCommandlineFromJSON("pr-manager-helper",
alias,
props)))
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 3a26876c10..6557711ec1 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -395,6 +395,9 @@ qemuMaybeBuildPRManagerInfoProps(virDomainObjPtr vm,
*propsret = NULL;
*aliasret = NULL;
+ if (!disk->src->pr)
+ return 0;
+
if (virStoragePRDefIsManaged(disk->src->pr) &&
priv->prDaemonRunning) {
/* @disk requires qemu-pr-helper but there's already one running. */
10:53 a.m.
New subject: [libvirt] [PATCH 10/13] qemu: command: Move check whether PR manager object props need to be built
On Mon, May 14, 2018 at 16:46:15 -0400, John Ferlan wrote:
On 05/14/2018 06:45 AM, Peter Krempa wrote:
> Move it out of the formatter function and let the caller decide this.
s/formatter/format/
I cannot recall our current consistency... Some times we seem to have
the lowest routine make the singular check and other times we have the
caller make the check.
>
> Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
> ---
> src/qemu/qemu_command.c | 9 +++------
> src/qemu/qemu_hotplug.c | 3 +++
> 2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 7df9979cb2..c38dde5a60 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -9687,9 +9687,6 @@ qemuBuildPRManagerInfoProps(const virDomainDiskDef *disk,
> *propsret = NULL;
> *aliasret = NULL;
>
> - if (!disk->src->pr)
> - return 0;
> -
> if (virStoragePRDefIsManaged(disk->src->pr)) {
> if (VIR_STRDUP(alias, qemuDomainGetManagedPRAlias()) < 0)
> goto cleanup;
> @@ -9725,6 +9722,9 @@ qemuBuildMasterPRCommandLine(virCommandPtr cmd,
> for (i = 0; i < def->ndisks; i++) {
> const virDomainDiskDef *disk = def->disks[i];
>
> + if (!disk->src->pr)
> + continue;
> +
> if (virStoragePRDefIsManaged(disk->src->pr)) {
> if (managedAdded)
> continue;
> @@ -9735,9 +9735,6 @@ qemuBuildMasterPRCommandLine(virCommandPtr cmd,
> if (qemuBuildPRManagerInfoProps(disk, &props, &alias) < 0)
> goto cleanup;
>
> - if (!props)
> - continue;
> -
This one seems unrelated... although at this point I'm not sure how it
could happen... I'd say keep it as is unless Michal had some other
reason or maybe remembers what it was there for originally.
It actually is related. Prior to this patch qemuBuildPRManagerInfoProps
either filled props or not depending on the configuration.
This patch is actually changing that so that it always returns props
and moves the duty of checking whether it is needed to the caller.
In the current situation props will ever be set to null when
qemuBuildPRManagerInfoProps returns -1 due to allocation failure.
5:45 a.m.
New subject: [libvirt] [PATCH 11/13] conf: domain: Add helper to check whether a domain def requires use of PR
Extract the lookup code so that it can be reused later.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/domain_conf.c | 21 +++++++++++++++++++++
src/conf/domain_conf.h | 3 +++
src/libvirt_private.syms | 1 +
src/qemu/qemu_process.c | 23 ++---------------------
4 files changed, 27 insertions(+), 21 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 86229db654..6f16e4ade4 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -29528,3 +29528,24 @@ virDomainDiskGetDetectZeroesMode(virDomainDiskDiscard discard,
return detect_zeroes;
}
+
+
+/**
+ * virDomainDefHasManagedPR:
+ * @def: domain definition
+ *
+ * Returns true if any of the domain disks requires the use of the managed
+ * persistent reservations infrastructure.
+ */
+bool
+virDomainDefHasManagedPR(const virDomainDef *def)
+{
+ size_t i;
+
+ for (i = 0; i < def->ndisks; i++) {
+ if (virStoragePRDefIsManaged(def->disks[i]->src->pr))
+ return true;
+ }
+
+ return false;
+}
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 07d04fb2f9..f1add06155 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -3543,4 +3543,7 @@ int
virDomainDiskGetDetectZeroesMode(virDomainDiskDiscard discard,
virDomainDiskDetectZeroes detect_zeroes);
+bool
+virDomainDefHasManagedPR(const virDomainDef *def);
+
#endif /* __DOMAIN_CONF_H */
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index dd10be9753..a0b78f72ba 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -275,6 +275,7 @@ virDomainDefGetVcpus;
virDomainDefGetVcpusMax;
virDomainDefGetVcpusTopology;
virDomainDefHasDeviceAddress;
+virDomainDefHasManagedPR;
virDomainDefHasMemballoon;
virDomainDefHasMemoryHotplug;
virDomainDefHasUSB;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index af29bcc59e..5b73a61962 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2748,26 +2748,6 @@ qemuProcessStartManagedPRDaemon(virDomainObjPtr vm)
}
-static int
-qemuProcessMaybeStartManagedPRDaemon(virDomainObjPtr vm)
-{
- bool hasManaged = false;
- size_t i;
-
- for (i = 0; i < vm->def->ndisks; i++) {
- if (virStoragePRDefIsManaged(vm->def->disks[i]->src->pr)) {
- hasManaged = true;
- break;
- }
- }
-
- if (!hasManaged)
- return 0;
-
- return qemuProcessStartManagedPRDaemon(vm);
-}
-
-
static int
qemuProcessInitPasswords(virQEMUDriverPtr driver,
virDomainObjPtr vm,
@@ -6285,7 +6265,8 @@ qemuProcessLaunch(virConnectPtr conn,
goto cleanup;
VIR_DEBUG("Setting up managed PR daemon");
- if (qemuProcessMaybeStartManagedPRDaemon(vm) < 0)
+ if (virDomainDefHasManagedPR(vm->def) &&
+ qemuProcessStartManagedPRDaemon(vm) < 0)
goto cleanup;
VIR_DEBUG("Setting domain security labels");
--
2.16.2
5:45 a.m.
New subject: [libvirt] [PATCH 12/13] util: storage: Store PR manager alias in the definition
Rather than always re-generating the alias store it in the definition
and in the status XML.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_command.c | 23 +++------------------
src/qemu/qemu_command.h | 3 +--
src/qemu/qemu_domain.c | 16 +++++++++++++--
src/qemu/qemu_hotplug.c | 34 ++++++++++---------------------
src/util/virstoragefile.c | 1 +
src/util/virstoragefile.h | 3 +++
tests/qemustatusxml2xmldata/modern-in.xml | 4 ++++
7 files changed, 37 insertions(+), 47 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index c38dde5a60..84d7d51c7c 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9669,7 +9669,6 @@ qemuBuildPanicCommandLine(virCommandPtr cmd,
* qemuBuildPRManagerInfoProps:
* @disk: disk definition
* @propsret: Returns JSON object containing properties of the pr-manager-helper object
- * @aliasret: alias of the pr-manager-helper object
*
* Build the JSON properties for the pr-manager object.
*
@@ -9678,32 +9677,19 @@ qemuBuildPanicCommandLine(virCommandPtr cmd,
*/
int
qemuBuildPRManagerInfoProps(const virDomainDiskDef *disk,
- virJSONValuePtr *propsret,
- char **aliasret)
+ virJSONValuePtr *propsret)
{
- char *alias = NULL;
int ret = -1;
*propsret = NULL;
- *aliasret = NULL;
-
- if (virStoragePRDefIsManaged(disk->src->pr)) {
- if (VIR_STRDUP(alias, qemuDomainGetManagedPRAlias()) < 0)
- goto cleanup;
- } else {
- if (!(alias = qemuDomainGetUnmanagedPRAlias(disk->info.alias)))
- goto cleanup;
- }
if (virJSONValueObjectCreate(propsret,
"s:path", disk->src->pr->path,
NULL) < 0)
goto cleanup;
- VIR_STEAL_PTR(*aliasret, alias);
ret = 0;
cleanup:
- VIR_FREE(alias);
return ret;
}
@@ -9715,7 +9701,6 @@ qemuBuildMasterPRCommandLine(virCommandPtr cmd,
size_t i;
bool managedAdded = false;
virJSONValuePtr props = NULL;
- char *alias = NULL;
char *tmp = NULL;
int ret = -1;
@@ -9732,14 +9717,13 @@ qemuBuildMasterPRCommandLine(virCommandPtr cmd,
managedAdded = true;
}
- if (qemuBuildPRManagerInfoProps(disk, &props, &alias) < 0)
+ if (qemuBuildPRManagerInfoProps(disk, &props) < 0)
goto cleanup;
if (!(tmp = virQEMUBuildObjectCommandlineFromJSON("pr-manager-helper",
- alias,
+
disk->src->pr->mgralias,
props)))
goto cleanup;
- VIR_FREE(alias);
virJSONValueFree(props);
props = NULL;
@@ -9749,7 +9733,6 @@ qemuBuildMasterPRCommandLine(virCommandPtr cmd,
ret = 0;
cleanup:
- VIR_FREE(alias);
virJSONValueFree(props);
return ret;
}
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index 621592cd79..28bc33558b 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -56,8 +56,7 @@ virCommandPtr qemuBuildCommandLine(virQEMUDriverPtr driver,
/* Generate the object properties for pr-manager */
int qemuBuildPRManagerInfoProps(const virDomainDiskDef *disk,
- virJSONValuePtr *propsret,
- char **alias);
+ virJSONValuePtr *propsret);
/* Generate the object properties for a secret */
int qemuBuildSecretInfoProps(qemuDomainSecretInfoPtr secinfo,
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 92217e66fe..1572ce5c2d 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1941,6 +1941,9 @@ qemuStorageSourcePrivateDataParse(xmlXPathContextPtr ctxt,
src->nodestorage =
virXPathString("string(./nodenames/nodename[@type='storage']/@name)",
ctxt);
src->nodeformat =
virXPathString("string(./nodenames/nodename[@type='format']/@name)",
ctxt);
+ if (src->pr)
+ src->pr->mgralias =
virXPathString("string(./reservations/@mgralias)", ctxt);
+
if (virStorageSourcePrivateDataParseRelPath(ctxt, src) < 0)
return -1;
@@ -1961,6 +1964,9 @@ qemuStorageSourcePrivateDataFormat(virStorageSourcePtr src,
virBufferAddLit(buf, "</nodenames>\n");
}
+ if (src->pr)
+ virBufferAsprintf(buf, "<reservations mgralias='%s'/>\n",
src->pr->mgralias);
+
if (virStorageSourcePrivateDataFormatRelPath(src, buf) < 0)
return -1;
@@ -11932,7 +11938,8 @@ qemuDomainPrepareDiskCachemode(virDomainDiskDefPtr disk)
static int
qemuDomainPrepareStorageSourcePR(virStorageSourcePtr src,
- qemuDomainObjPrivatePtr priv)
+ qemuDomainObjPrivatePtr priv,
+ const char *parentalias)
{
if (!src->pr)
return 0;
@@ -11940,6 +11947,11 @@ qemuDomainPrepareStorageSourcePR(virStorageSourcePtr src,
if (virStoragePRDefIsManaged(src->pr)) {
if (!(src->pr->path = qemuDomainGetManagedPRSocketPath(priv)))
return -1;
+ if (VIR_STRDUP(src->pr->mgralias, qemuDomainGetManagedPRAlias()) < 0)
+ return -1;
+ } else {
+ if (!(src->pr->mgralias = qemuDomainGetUnmanagedPRAlias(parentalias)))
+ return -1;
}
return 0;
@@ -11962,7 +11974,7 @@ qemuDomainPrepareDiskSource(virDomainDiskDefPtr disk,
if (qemuDomainPrepareDiskSourceChain(disk, NULL, cfg, priv->qemuCaps) < 0)
return -1;
- if (qemuDomainPrepareStorageSourcePR(disk->src, priv) < 0)
+ if (qemuDomainPrepareStorageSourcePR(disk->src, priv, disk->info.alias) <
0)
return -1;
return 0;
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 6557711ec1..9481123c19 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -387,13 +387,11 @@ qemuDomainMaybeStartPRDaemon(virDomainObjPtr vm,
static int
qemuMaybeBuildPRManagerInfoProps(virDomainObjPtr vm,
const virDomainDiskDef *disk,
- virJSONValuePtr *propsret,
- char **aliasret)
+ virJSONValuePtr *propsret)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
*propsret = NULL;
- *aliasret = NULL;
if (!disk->src->pr)
return 0;
@@ -404,7 +402,7 @@ qemuMaybeBuildPRManagerInfoProps(virDomainObjPtr vm,
return 0;
}
- return qemuBuildPRManagerInfoProps(disk, propsret, aliasret);
+ return qemuBuildPRManagerInfoProps(disk, propsret);
}
@@ -425,7 +423,6 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
char *devstr = NULL;
char *drivestr = NULL;
char *drivealias = NULL;
- char *prmgrAlias = NULL;
bool driveAdded = false;
bool secobjAdded = false;
bool encobjAdded = false;
@@ -462,7 +459,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
if (encinfo && qemuBuildSecretInfoProps(encinfo, &encobjProps) < 0)
goto error;
- if (qemuMaybeBuildPRManagerInfoProps(vm, disk, &prmgrProps, &prmgrAlias) <
0)
+ if (qemuMaybeBuildPRManagerInfoProps(vm, disk, &prmgrProps) < 0)
goto error;
/* Start daemon only after prmgrProps is built. Otherwise
@@ -511,7 +508,8 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
}
if (prmgrProps) {
- rv = qemuMonitorAddObject(priv->mon, "pr-manager-helper",
prmgrAlias,
+ rv = qemuMonitorAddObject(priv->mon, "pr-manager-helper",
+ disk->src->pr->mgralias,
prmgrProps);
prmgrProps = NULL; /* qemuMonitorAddObject consumes */
if (rv < 0)
@@ -541,7 +539,6 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
virJSONValueFree(encobjProps);
virJSONValueFree(secobjProps);
qemuDomainSecretDiskDestroy(disk);
- VIR_FREE(prmgrAlias);
VIR_FREE(drivealias);
VIR_FREE(drivestr);
VIR_FREE(devstr);
@@ -559,7 +556,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
if (encobjAdded)
ignore_value(qemuMonitorDelObject(priv->mon, encinfo->s.aes.alias));
if (prmgrAdded)
- ignore_value(qemuMonitorDelObject(priv->mon, prmgrAlias));
+ ignore_value(qemuMonitorDelObject(priv->mon,
disk->src->pr->mgralias));
if (qemuDomainObjExitMonitor(driver, vm) < 0)
ret = -2;
virErrorRestore(&orig_err);
@@ -3832,22 +3829,18 @@ static bool qemuIsMultiFunctionDevice(virDomainDefPtr def,
static int
qemuDomainDiskNeedRemovePR(virDomainObjPtr vm,
virDomainDiskDefPtr disk,
- char **aliasret,
bool *stopDaemon)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
size_t i;
- *aliasret = NULL;
*stopDaemon = false;
if (!disk->src->pr)
return 0;
- if (!virStoragePRDefIsManaged(disk->src->pr)) {
- *aliasret = qemuDomainGetUnmanagedPRAlias(disk->info.alias);
- return *aliasret ? 0 : -1;
- }
+ if (!virStoragePRDefIsManaged(disk->src->pr))
+ return 0;
for (i = 0; i < vm->def->ndisks; i++) {
const virDomainDiskDef *domainDisk = vm->def->disks[i];
@@ -3862,9 +3855,6 @@ qemuDomainDiskNeedRemovePR(virDomainObjPtr vm,
if (i != vm->def->ndisks)
return 0;
- if (VIR_STRDUP(*aliasret, qemuDomainGetManagedPRAlias()) < 0)
- return -1;
-
if (priv->prDaemonRunning)
*stopDaemon = true;
@@ -3885,7 +3875,6 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
char *drivestr;
char *objAlias = NULL;
char *encAlias = NULL;
- char *prmgrAlias = NULL;
bool stopPRDaemon = false;
VIR_DEBUG("Removing disk %s from domain %p %s",
@@ -3924,7 +3913,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
}
}
- if (qemuDomainDiskNeedRemovePR(vm, disk, &prmgrAlias, &stopPRDaemon) < 0)
+ if (qemuDomainDiskNeedRemovePR(vm, disk, &stopPRDaemon) < 0)
return -1;
qemuDomainObjEnterMonitor(driver, vm);
@@ -3943,9 +3932,8 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
VIR_FREE(encAlias);
/* If it fails, then so be it - it was a best shot */
- if (prmgrAlias)
- ignore_value(qemuMonitorDelObject(priv->mon, prmgrAlias));
- VIR_FREE(prmgrAlias);
+ if (disk->src->pr)
+ ignore_value(qemuMonitorDelObject(priv->mon,
disk->src->pr->mgralias));
if (disk->src->haveTLS)
ignore_value(qemuMonitorDelObject(priv->mon, disk->src->tlsAlias));
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index dbbe758f30..54de2c1c30 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1899,6 +1899,7 @@ virStoragePRDefFree(virStoragePRDefPtr prd)
return;
VIR_FREE(prd->path);
+ VIR_FREE(prd->mgralias);
VIR_FREE(prd);
}
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index 3a90c60fa5..1631c4cf66 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -221,6 +221,9 @@ typedef virStoragePRDef *virStoragePRDefPtr;
struct _virStoragePRDef {
int managed; /* enum virTristateBool */
char *path;
+
+ /* manager object alias */
+ char *mgralias;
};
typedef struct _virStorageDriverData virStorageDriverData;
diff --git a/tests/qemustatusxml2xmldata/modern-in.xml
b/tests/qemustatusxml2xmldata/modern-in.xml
index d57e1f605f..d63fcf79f1 100644
--- a/tests/qemustatusxml2xmldata/modern-in.xml
+++ b/tests/qemustatusxml2xmldata/modern-in.xml
@@ -308,11 +308,15 @@
<backingStore type='file' index='1'>
<format type='qcow2'/>
<source file='/var/lib/libvirt/images/base.qcow2'>
+ <reservations managed='yes'>
+ <source type='unix' path='/somepath/ux.sck'
mode='client'/>
+ </reservations>
<privateData>
<nodenames>
<nodename type='storage' name='test-storage'/>
<nodename type='format' name='test-format'/>
</nodenames>
+ <reservations mgralias='test-alias'/>
<relPath>base.qcow2</relPath>
</privateData>
</source>
--
2.16.2
10:19 a.m.
New subject: [libvirt] [PATCH 12/13] util: storage: Store PR manager alias in the definition
On 05/14/2018 12:45 PM, Peter Krempa wrote:
Rather than always re-generating the alias store it in the
definition
and in the status XML.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_command.c | 23 +++------------------
src/qemu/qemu_command.h | 3 +--
src/qemu/qemu_domain.c | 16 +++++++++++++--
src/qemu/qemu_hotplug.c | 34 ++++++++++---------------------
src/util/virstoragefile.c | 1 +
src/util/virstoragefile.h | 3 +++
tests/qemustatusxml2xmldata/modern-in.xml | 4 ++++
7 files changed, 37 insertions(+), 47 deletions(-)
Yes, this makes sense to me. I've kept alias in status XML for all the
versions until the very last one. You have my ACK, but since John was
opposed maybe we should ask for his opinion too (so that we don't go
behind his back).
Michal
5:03 p.m.
New subject: [libvirt] [PATCH 12/13] util: storage: Store PR manager alias in the definition
On 05/14/2018 11:19 AM, Michal Privoznik wrote:
On 05/14/2018 12:45 PM, Peter Krempa wrote:
> Rather than always re-generating the alias store it in the definition
> and in the status XML.
>
> Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
> ---
> src/qemu/qemu_command.c | 23 +++------------------
> src/qemu/qemu_command.h | 3 +--
> src/qemu/qemu_domain.c | 16 +++++++++++++--
> src/qemu/qemu_hotplug.c | 34 ++++++++++---------------------
> src/util/virstoragefile.c | 1 +
> src/util/virstoragefile.h | 3 +++
> tests/qemustatusxml2xmldata/modern-in.xml | 4 ++++
> 7 files changed, 37 insertions(+), 47 deletions(-)
Yes, this makes sense to me. I've kept alias in status XML for all the
versions until the very last one. You have my ACK, but since John was
opposed maybe we should ask for his opinion too (so that we don't go
behind his back).
I still see no purpose for an alias to be saved since it's static, but I
seem to be alone in that thinking. Just as much as I was alone in the
thinking that qemuDomainGetManagedPRAlias is unnecessary and that a
constant static string would work just the same. I'm also not convinced
that a non managed system should be supported, but I recall Michal
noting something during one of the reviews that it was useful for some
future work.
Shouldn't at the very least the formatting of the path and alias only
occur for !(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE)... Would mean
passing @flags to virStoragePRDefFormat. At the very least it'll make
it obvious about it's only being formatted for the active/status guest.
John
11 a.m.
New subject: [libvirt] [PATCH 12/13] util: storage: Store PR manager alias in the definition
On Mon, May 14, 2018 at 18:03:00 -0400, John Ferlan wrote:
On 05/14/2018 11:19 AM, Michal Privoznik wrote:
> On 05/14/2018 12:45 PM, Peter Krempa wrote:
>> Rather than always re-generating the alias store it in the definition
>> and in the status XML.
>>
>> Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
>> ---
>> src/qemu/qemu_command.c | 23 +++------------------
>> src/qemu/qemu_command.h | 3 +--
>> src/qemu/qemu_domain.c | 16 +++++++++++++--
>> src/qemu/qemu_hotplug.c | 34 ++++++++++---------------------
>> src/util/virstoragefile.c | 1 +
>> src/util/virstoragefile.h | 3 +++
>> tests/qemustatusxml2xmldata/modern-in.xml | 4 ++++
>> 7 files changed, 37 insertions(+), 47 deletions(-)
>
> Yes, this makes sense to me. I've kept alias in status XML for all the
> versions until the very last one. You have my ACK, but since John was
> opposed maybe we should ask for his opinion too (so that we don't go
> behind his back).
>
I still see no purpose for an alias to be saved since it's static, but I
seem to be alone in that thinking. Just as much as I was alone in the
thinking that qemuDomainGetManagedPRAlias is unnecessary and that a
constant static string would work just the same. I'm also not convinced
that a non managed system should be supported, but I recall Michal
noting something during one of the reviews that it was useful for some
future work.
The point of storing the aliases in the XML is that it records the state
as we've used when we've created the object in qemu. If we will need to
change the naming scheme for some reason we'd require to add a lot of
code which will use the old/new alias depending on which would be
originally used. By storing it into the status XML you are relieved of
all this by just using what was used before.
The future work referenced by Michal is the blockdev work, where the
hot-remove code paths for disks will need to use the correct alias for
the individual layer of the backing chain rather than the disk.
I'll need to tweak this for the Authentication/encryption/TLS secrets as
well, since they can't all share the same alias even when they
correspond to the same disk.
Shouldn't at the very least the formatting of the path and alias
only
occur for !(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE)... Would mean
passing @flags to virStoragePRDefFormat. At the very least it'll make
it obvious about it's only being formatted for the active/status guest.
The alias is formatted in the <privateData> section of the
virStorageSource which is only formatted when the XML is active. That is
handled by the global storage source private data formatter.
5:45 a.m.
New subject: [libvirt] [PATCH 13/13] qemu: hotplug: Replace qemuDomainDiskNeedRemovePR
The function can be replaced by much simpler logic.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 44 +++-----------------------------------------
1 file changed, 3 insertions(+), 41 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 9481123c19..96042bc06c 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3826,42 +3826,6 @@ static bool qemuIsMultiFunctionDevice(virDomainDefPtr def,
}
-static int
-qemuDomainDiskNeedRemovePR(virDomainObjPtr vm,
- virDomainDiskDefPtr disk,
- bool *stopDaemon)
-{
- qemuDomainObjPrivatePtr priv = vm->privateData;
- size_t i;
-
- *stopDaemon = false;
-
- if (!disk->src->pr)
- return 0;
-
- if (!virStoragePRDefIsManaged(disk->src->pr))
- return 0;
-
- for (i = 0; i < vm->def->ndisks; i++) {
- const virDomainDiskDef *domainDisk = vm->def->disks[i];
-
- if (domainDisk == disk)
- continue;
-
- if (virStoragePRDefIsManaged(domainDisk->src->pr))
- break;
- }
-
- if (i != vm->def->ndisks)
- return 0;
-
- if (priv->prDaemonRunning)
- *stopDaemon = true;
-
- return 0;
-}
-
-
static int
qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
virDomainObjPtr vm,
@@ -3875,7 +3839,6 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
char *drivestr;
char *objAlias = NULL;
char *encAlias = NULL;
- bool stopPRDaemon = false;
VIR_DEBUG("Removing disk %s from domain %p %s",
disk->info.alias, vm, vm->def->name);
@@ -3913,9 +3876,6 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
}
}
- if (qemuDomainDiskNeedRemovePR(vm, disk, &stopPRDaemon) < 0)
- return -1;
-
qemuDomainObjEnterMonitor(driver, vm);
qemuMonitorDriveDel(priv->mon, drivestr);
@@ -3953,7 +3913,9 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
}
}
- if (stopPRDaemon)
+ /* check if the last disk with managed PR was just removed */
+ if (priv->prDaemonRunning &&
+ !virDomainDefHasManagedPR(vm->def))
qemuProcessKillManagedPRDaemon(vm);
qemuDomainReleaseDeviceAddress(vm, &disk->info, src);
--
2.16.2
10:19 a.m.
On 05/14/2018 12:41 PM, Peter Krempa wrote:
The XML design for the PR stuff is slightly weird so fix it and
refactor
the code so that it will be much easier to use it with the blockdev
infrastructure.
Peter Krempa (13):
qemu: hotplug: Fix spacing around addition operator
qemu: alias: Allow passing alias of parent when generating PR manager
alias
qemu: command: Fix comment for qemuBuildPRManagerInfoProps
qemu: Move validation of PR manager support
util: storage: Drop pointless 'enabled' form PR definition
util: storage: Drop virStoragePRDefIsEnabled
util: storage: Allow passing <source> also for managed PR case
qemu: Assign managed PR path when preparing storage source
qemu: process: Change semantics of functions starting PR daemon
qemu: command: Move check whether PR manager object props need to be
built
conf: domain: Add helper to check whether a domain def requires use of
PR
util: storage: Store PR manager alias in the definition
qemu: hotplug: Replace qemuDomainDiskNeedRemovePR
docs/formatdomain.html.in | 21 ++--
docs/schemas/storagecommon.rng | 3 -
src/conf/domain_conf.c | 21 ++++
src/conf/domain_conf.h | 3 +
src/libvirt_private.syms | 2 +-
src/qemu/qemu_alias.c | 4 +-
src/qemu/qemu_alias.h | 2 +-
src/qemu/qemu_command.c | 61 +++-------
src/qemu/qemu_command.h | 6 +-
src/qemu/qemu_domain.c | 66 ++++++++---
src/qemu/qemu_domain.h | 3 +-
src/qemu/qemu_hotplug.c | 83 +++-----------
src/qemu/qemu_process.c | 40 ++-----
src/qemu/qemu_process.h | 5 +-
src/util/virstoragefile.c | 124 ++++++++-------------
src/util/virstoragefile.h | 5 +-
tests/qemustatusxml2xmldata/modern-in.xml | 4 +
.../disk-virtio-scsi-reservations.xml | 4 +-
tests/qemuxml2xmltest.c | 2 +-
19 files changed, 191 insertions(+), 268 deletions(-)
You have my ACK on the series, but I guess we'll need John's input on
12/13 because he was opposed to saving anything in status XML.
Michal
2384
days inactive
2385
days old
26 comments
3 participants
participants (3)
-
John Ferlan -
Michal Privoznik -
Peter Krempa