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