[libvirt] [PATCH 0/6] qemu: domain: Private data handling refactors
Extracted from a huge work-in-progres series since these look stand-alone. Peter Krempa (6): qemu: Add qemu functions for storage source private data handling qemu: domain: Split out formating of Job data from private data formatter qemu: domain: Don't overwrite job type in private data qemu: domain: Return early in qemuDomainObjPrivateXMLFormatJob qemu: domain: Use virXMLFormatElement in qemuDomainObjPrivateXMLFormatJob qemu: domain: Extract parsing of job-related private XML src/qemu/qemu_domain.c | 218 ++++++++++++++++++++++++++++++------------------- 1 file changed, 134 insertions(+), 84 deletions(-) -- 2.16.2
The qemu driver registered the helpers from util code, but it will be necessary to format also some qemu-specific data. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 8b4efc82de..dabc78e6bf 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2530,6 +2530,28 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, } +static int +qemuStorageSourcePrivateDataParse(xmlXPathContextPtr ctxt, + virStorageSourcePtr src) +{ + if (virStorageSourcePrivateDataParseRelPath(ctxt, src) < 0) + return -1; + + return 0; +} + + +static int +qemuStorageSourcePrivateDataFormat(virStorageSourcePtr src, + virBufferPtr buf) +{ + if (virStorageSourcePrivateDataFormatRelPath(src, buf) < 0) + return -1; + + return 0; +} + + virDomainXMLPrivateDataCallbacks virQEMUDriverPrivateDataCallbacks = { .alloc = qemuDomainObjPrivateAlloc, .free = qemuDomainObjPrivateFree, @@ -2538,8 +2560,8 @@ virDomainXMLPrivateDataCallbacks virQEMUDriverPrivateDataCallbacks = { .chrSourceNew = qemuDomainChrSourcePrivateNew, .parse = qemuDomainObjPrivateXMLParse, .format = qemuDomainObjPrivateXMLFormat, - .storageParse = virStorageSourcePrivateDataParseRelPath, - .storageFormat = virStorageSourcePrivateDataFormatRelPath, + .storageParse = qemuStorageSourcePrivateDataParse, + .storageFormat = qemuStorageSourcePrivateDataFormat, }; -- 2.16.2
Separate the code for later refactoring Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 84 ++++++++++++++++++++++++++++---------------------- 1 file changed, 47 insertions(+), 37 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index dabc78e6bf..2db736fbed 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2035,13 +2035,58 @@ qemuDomainObjPrivateXMLFormatAllowReboot(virBufferPtr buf, } +static void +qemuDomainObjPrivateXMLFormatJob(virBufferPtr buf, + virDomainObjPtr vm, + qemuDomainObjPrivatePtr priv) +{ + qemuDomainJob job; + + job = priv->job.active; + if (!qemuDomainTrackJob(job)) + priv->job.active = QEMU_JOB_NONE; + + if (priv->job.active || priv->job.asyncJob) { + virBufferAsprintf(buf, "<job type='%s' async='%s'", + qemuDomainJobTypeToString(priv->job.active), + qemuDomainAsyncJobTypeToString(priv->job.asyncJob)); + if (priv->job.phase) { + virBufferAsprintf(buf, " phase='%s'", + qemuDomainAsyncJobPhaseToString( + priv->job.asyncJob, priv->job.phase)); + } + if (priv->job.asyncJob != QEMU_ASYNC_JOB_MIGRATION_OUT) { + virBufferAddLit(buf, "/>\n"); + } else { + size_t i; + virDomainDiskDefPtr disk; + qemuDomainDiskPrivatePtr diskPriv; + + virBufferAddLit(buf, ">\n"); + virBufferAdjustIndent(buf, 2); + + for (i = 0; i < vm->def->ndisks; i++) { + disk = vm->def->disks[i]; + diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); + virBufferAsprintf(buf, "<disk dev='%s' migrating='%s'/>\n", + disk->dst, + diskPriv->migrating ? "yes" : "no"); + } + + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</job>\n"); + } + } + priv->job.active = job; +} + + static int qemuDomainObjPrivateXMLFormat(virBufferPtr buf, virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; const char *monitorpath; - qemuDomainJob job; /* priv->monitor_chr is set only for qemu */ if (priv->monConfig) { @@ -2092,42 +2137,7 @@ qemuDomainObjPrivateXMLFormat(virBufferPtr buf, if (priv->lockState) virBufferAsprintf(buf, "<lockstate>%s</lockstate>\n", priv->lockState); - job = priv->job.active; - if (!qemuDomainTrackJob(job)) - priv->job.active = QEMU_JOB_NONE; - - if (priv->job.active || priv->job.asyncJob) { - virBufferAsprintf(buf, "<job type='%s' async='%s'", - qemuDomainJobTypeToString(priv->job.active), - qemuDomainAsyncJobTypeToString(priv->job.asyncJob)); - if (priv->job.phase) { - virBufferAsprintf(buf, " phase='%s'", - qemuDomainAsyncJobPhaseToString( - priv->job.asyncJob, priv->job.phase)); - } - if (priv->job.asyncJob != QEMU_ASYNC_JOB_MIGRATION_OUT) { - virBufferAddLit(buf, "/>\n"); - } else { - size_t i; - virDomainDiskDefPtr disk; - qemuDomainDiskPrivatePtr diskPriv; - - virBufferAddLit(buf, ">\n"); - virBufferAdjustIndent(buf, 2); - - for (i = 0; i < vm->def->ndisks; i++) { - disk = vm->def->disks[i]; - diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); - virBufferAsprintf(buf, "<disk dev='%s' migrating='%s'/>\n", - disk->dst, - diskPriv->migrating ? "yes" : "no"); - } - - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</job>\n"); - } - } - priv->job.active = job; + qemuDomainObjPrivateXMLFormatJob(buf, vm, priv); if (priv->fakeReboot) virBufferAddLit(buf, "<fakereboot/>\n"); -- 2.16.2
The code overwrote the internal job type and then fixed it back. Since the job type is not accessed in the code this does not make much sense. Use the temporary value instead. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 2db736fbed..e5b494fa2a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2040,15 +2040,14 @@ qemuDomainObjPrivateXMLFormatJob(virBufferPtr buf, virDomainObjPtr vm, qemuDomainObjPrivatePtr priv) { - qemuDomainJob job; + qemuDomainJob job = priv->job.active; - job = priv->job.active; if (!qemuDomainTrackJob(job)) - priv->job.active = QEMU_JOB_NONE; + job = QEMU_JOB_NONE; - if (priv->job.active || priv->job.asyncJob) { + if (job || priv->job.asyncJob) { virBufferAsprintf(buf, "<job type='%s' async='%s'", - qemuDomainJobTypeToString(priv->job.active), + qemuDomainJobTypeToString(job), qemuDomainAsyncJobTypeToString(priv->job.asyncJob)); if (priv->job.phase) { virBufferAsprintf(buf, " phase='%s'", @@ -2077,7 +2076,6 @@ qemuDomainObjPrivateXMLFormatJob(virBufferPtr buf, virBufferAddLit(buf, "</job>\n"); } } - priv->job.active = job; } -- 2.16.2
Remove one level of nesting by returing early. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 54 ++++++++++++++++++++++++++------------------------ 1 file changed, 28 insertions(+), 26 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e5b494fa2a..b7fb9f264d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2045,36 +2045,38 @@ qemuDomainObjPrivateXMLFormatJob(virBufferPtr buf, if (!qemuDomainTrackJob(job)) job = QEMU_JOB_NONE; - if (job || priv->job.asyncJob) { - virBufferAsprintf(buf, "<job type='%s' async='%s'", - qemuDomainJobTypeToString(job), - qemuDomainAsyncJobTypeToString(priv->job.asyncJob)); - if (priv->job.phase) { - virBufferAsprintf(buf, " phase='%s'", - qemuDomainAsyncJobPhaseToString( - priv->job.asyncJob, priv->job.phase)); - } - if (priv->job.asyncJob != QEMU_ASYNC_JOB_MIGRATION_OUT) { - virBufferAddLit(buf, "/>\n"); - } else { - size_t i; - virDomainDiskDefPtr disk; - qemuDomainDiskPrivatePtr diskPriv; + if (job == QEMU_JOB_NONE && + priv->job.asyncJob == QEMU_ASYNC_JOB_NONE) + return; - virBufferAddLit(buf, ">\n"); - virBufferAdjustIndent(buf, 2); + virBufferAsprintf(buf, "<job type='%s' async='%s'", + qemuDomainJobTypeToString(job), + qemuDomainAsyncJobTypeToString(priv->job.asyncJob)); + if (priv->job.phase) { + virBufferAsprintf(buf, " phase='%s'", + qemuDomainAsyncJobPhaseToString(priv->job.asyncJob, + priv->job.phase)); + } + if (priv->job.asyncJob != QEMU_ASYNC_JOB_MIGRATION_OUT) { + virBufferAddLit(buf, "/>\n"); + } else { + size_t i; + virDomainDiskDefPtr disk; + qemuDomainDiskPrivatePtr diskPriv; - for (i = 0; i < vm->def->ndisks; i++) { - disk = vm->def->disks[i]; - diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); - virBufferAsprintf(buf, "<disk dev='%s' migrating='%s'/>\n", - disk->dst, - diskPriv->migrating ? "yes" : "no"); - } + virBufferAddLit(buf, ">\n"); + virBufferAdjustIndent(buf, 2); - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</job>\n"); + for (i = 0; i < vm->def->ndisks; i++) { + disk = vm->def->disks[i]; + diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); + virBufferAsprintf(buf, "<disk dev='%s' migrating='%s'/>\n", + disk->dst, + diskPriv->migrating ? "yes" : "no"); } + + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</job>\n"); } } -- 2.16.2
Modernize the code by using the clever formatter rather than checking manually when to format the end of the element. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index b7fb9f264d..e4088665ee 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2035,11 +2035,13 @@ qemuDomainObjPrivateXMLFormatAllowReboot(virBufferPtr buf, } -static void +static int qemuDomainObjPrivateXMLFormatJob(virBufferPtr buf, virDomainObjPtr vm, qemuDomainObjPrivatePtr priv) { + virBuffer attrBuf = VIR_BUFFER_INITIALIZER; + virBuffer childBuf = VIR_BUFFER_INITIALIZER; qemuDomainJob job = priv->job.active; if (!qemuDomainTrackJob(job)) @@ -2047,37 +2049,34 @@ qemuDomainObjPrivateXMLFormatJob(virBufferPtr buf, if (job == QEMU_JOB_NONE && priv->job.asyncJob == QEMU_ASYNC_JOB_NONE) - return; + return 0; + + virBufferSetChildIndent(&childBuf, buf); - virBufferAsprintf(buf, "<job type='%s' async='%s'", + virBufferAsprintf(&attrBuf, "type='%s' async='%s'", qemuDomainJobTypeToString(job), qemuDomainAsyncJobTypeToString(priv->job.asyncJob)); + if (priv->job.phase) { - virBufferAsprintf(buf, " phase='%s'", + virBufferAsprintf(&attrBuf, " phase='%s'", qemuDomainAsyncJobPhaseToString(priv->job.asyncJob, priv->job.phase)); } - if (priv->job.asyncJob != QEMU_ASYNC_JOB_MIGRATION_OUT) { - virBufferAddLit(buf, "/>\n"); - } else { + + if (priv->job.asyncJob == QEMU_ASYNC_JOB_MIGRATION_OUT) { size_t i; virDomainDiskDefPtr disk; qemuDomainDiskPrivatePtr diskPriv; - virBufferAddLit(buf, ">\n"); - virBufferAdjustIndent(buf, 2); - for (i = 0; i < vm->def->ndisks; i++) { disk = vm->def->disks[i]; diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); - virBufferAsprintf(buf, "<disk dev='%s' migrating='%s'/>\n", - disk->dst, - diskPriv->migrating ? "yes" : "no"); + virBufferAsprintf(&childBuf, "<disk dev='%s' migrating='%s'/>\n", + disk->dst, diskPriv->migrating ? "yes" : "no"); } - - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</job>\n"); } + + return virXMLFormatElement(buf, "job", &attrBuf, &childBuf); } @@ -2137,7 +2136,8 @@ qemuDomainObjPrivateXMLFormat(virBufferPtr buf, if (priv->lockState) virBufferAsprintf(buf, "<lockstate>%s</lockstate>\n", priv->lockState); - qemuDomainObjPrivateXMLFormatJob(buf, vm, priv); + if (qemuDomainObjPrivateXMLFormatJob(buf, vm, priv) < 0) + return -1; if (priv->fakeReboot) virBufferAddLit(buf, "<fakereboot/>\n"); -- 2.16.2
On Thu, Mar 01, 2018 at 18:59:46 +0100, Peter Krempa wrote:
Modernize the code by using the clever formatter rather than checking manually when to format the end of the element.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index b7fb9f264d..e4088665ee 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2035,11 +2035,13 @@ qemuDomainObjPrivateXMLFormatAllowReboot(virBufferPtr buf, }
-static void +static int qemuDomainObjPrivateXMLFormatJob(virBufferPtr buf, virDomainObjPtr vm, qemuDomainObjPrivatePtr priv) { + virBuffer attrBuf = VIR_BUFFER_INITIALIZER; + virBuffer childBuf = VIR_BUFFER_INITIALIZER; qemuDomainJob job = priv->job.active;
if (!qemuDomainTrackJob(job)) @@ -2047,37 +2049,34 @@ qemuDomainObjPrivateXMLFormatJob(virBufferPtr buf,
if (job == QEMU_JOB_NONE && priv->job.asyncJob == QEMU_ASYNC_JOB_NONE) - return; + return 0; + + virBufferSetChildIndent(&childBuf, buf);
- virBufferAsprintf(buf, "<job type='%s' async='%s'", + virBufferAsprintf(&attrBuf, "type='%s' async='%s'",
s/"type/" type/ here otherwise they'd be squashed together. It was found by a test-suite improvement that I'll post shortly.
Similarly to the formatter extract the parser code. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 108 ++++++++++++++++++++++++++++--------------------- 1 file changed, 63 insertions(+), 45 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e4088665ee..a6521d5f94 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2304,6 +2304,68 @@ qemuDomainObjPrivateXMLParseAllowReboot(xmlXPathContextPtr ctxt, } +static int +qemuDomainObjPrivateXMLParseJob(virDomainObjPtr vm, + qemuDomainObjPrivatePtr priv, + xmlXPathContextPtr ctxt) +{ + xmlNodePtr *nodes = NULL; + char *tmp = NULL; + size_t i; + int n; + int ret = -1; + + if ((tmp = virXPathString("string(./job[1]/@async)", ctxt))) { + int async; + + if ((async = qemuDomainAsyncJobTypeFromString(tmp)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown async job type %s"), tmp); + goto cleanup; + } + VIR_FREE(tmp); + priv->job.asyncJob = async; + + if ((tmp = virXPathString("string(./job[1]/@phase)", ctxt))) { + priv->job.phase = qemuDomainAsyncJobPhaseFromString(async, tmp); + if (priv->job.phase < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown job phase %s"), tmp); + goto cleanup; + } + VIR_FREE(tmp); + } + } + + if ((n = virXPathNodeSet("./job[1]/disk[@migrating='yes']", ctxt, &nodes)) < 0) + goto cleanup; + + if (n > 0) { + if (priv->job.asyncJob != QEMU_ASYNC_JOB_MIGRATION_OUT) { + VIR_WARN("Found disks marked for migration but we were not " + "migrating"); + n = 0; + } + for (i = 0; i < n; i++) { + char *dst = virXMLPropString(nodes[i], "dev"); + virDomainDiskDefPtr disk; + + if (dst && (disk = virDomainDiskByName(vm->def, dst, false))) + QEMU_DOMAIN_DISK_PRIVATE(disk)->migrating = true; + VIR_FREE(dst); + } + } + VIR_FREE(nodes); + + ret = 0; + + cleanup: + VIR_FREE(tmp); + VIR_FREE(nodes); + return ret; +} + + static int qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, virDomainObjPtr vm, @@ -2431,52 +2493,8 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, priv->job.active = type; } - if ((tmp = virXPathString("string(./job[1]/@async)", ctxt))) { - int async; - - if ((async = qemuDomainAsyncJobTypeFromString(tmp)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unknown async job type %s"), tmp); - VIR_FREE(tmp); - goto error; - } - VIR_FREE(tmp); - priv->job.asyncJob = async; - - if ((tmp = virXPathString("string(./job[1]/@phase)", ctxt))) { - priv->job.phase = qemuDomainAsyncJobPhaseFromString(async, tmp); - if (priv->job.phase < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unknown job phase %s"), tmp); - VIR_FREE(tmp); - goto error; - } - VIR_FREE(tmp); - } - } - - if ((n = virXPathNodeSet("./job[1]/disk[@migrating='yes']", - ctxt, &nodes)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to parse list of disks marked for migration")); + if (qemuDomainObjPrivateXMLParseJob(vm, priv, ctxt) < 0) goto error; - } - if (n > 0) { - if (priv->job.asyncJob != QEMU_ASYNC_JOB_MIGRATION_OUT) { - VIR_WARN("Found disks marked for migration but we were not " - "migrating"); - n = 0; - } - for (i = 0; i < n; i++) { - char *dst = virXMLPropString(nodes[i], "dev"); - virDomainDiskDefPtr disk; - - if (dst && (disk = virDomainDiskByName(vm->def, dst, false))) - QEMU_DOMAIN_DISK_PRIVATE(disk)->migrating = true; - VIR_FREE(dst); - } - } - VIR_FREE(nodes); priv->fakeReboot = virXPathBoolean("boolean(./fakereboot)", ctxt) == 1; -- 2.16.2
On 03/01/2018 12:59 PM, Peter Krempa wrote:
Extracted from a huge work-in-progres series since these look stand-alone.
Peter Krempa (6): qemu: Add qemu functions for storage source private data handling qemu: domain: Split out formating of Job data from private data formatter qemu: domain: Don't overwrite job type in private data qemu: domain: Return early in qemuDomainObjPrivateXMLFormatJob qemu: domain: Use virXMLFormatElement in qemuDomainObjPrivateXMLFormatJob qemu: domain: Extract parsing of job-related private XML
src/qemu/qemu_domain.c | 218 ++++++++++++++++++++++++++++++------------------- 1 file changed, 134 insertions(+), 84 deletions(-)
Series, Reviewed-by: John Ferlan <jferlan@redhat.com> BTW: Your patch 1 has some similarities to Michal's patch 5 for the persistent reservations.
participants (2)
-
John Ferlan -
Peter Krempa