[PATCH 0/2] Move generation of NVDIMM UUID into post parse callback
I've noticed this while working on virtio-pmem-pci patches. Michal Prívozník (2): conf: Turn @uuid member of _virDomainMemoryDef struct into a pointer conf: Move generation of NVDIMM UUID into post parse callback src/conf/domain_conf.c | 47 +++++++++++++++++++++++++---------------- src/conf/domain_conf.h | 2 +- src/qemu/qemu_command.c | 2 +- 3 files changed, 31 insertions(+), 20 deletions(-) -- 2.26.2
The _virDomainMemoryDef structure has @uuid member which is needed for PPC64 guests. No other architectures use it. Since the member is VIR_UUID_BUFLEN bytes long, the structure is unnecessary big. If the member is just a pointer then we can also replace some calls of virUUIDIsValid() with plain test against NULL. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 14 ++++++++------ src/conf/domain_conf.h | 2 +- src/qemu/qemu_command.c | 2 +- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ff3b7cbfc8..a2ddfcf947 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3116,6 +3116,7 @@ void virDomainMemoryDefFree(virDomainMemoryDefPtr def) VIR_FREE(def->nvdimmPath); virBitmapFree(def->sourceNodes); + VIR_FREE(def->uuid); virDomainDeviceInfoClear(&def->info); VIR_FREE(def); } @@ -15530,6 +15531,7 @@ virDomainMemoryDefParseXML(virDomainXMLOptionPtr xmlopt, /* Extract nvdimm uuid or generate a new one */ tmp = virXPathString("string(./uuid[1])", ctxt); + def->uuid = g_new0(unsigned char, VIR_UUID_BUFLEN); if (!tmp) { if (virUUIDGenerate(def->uuid) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -22808,7 +22810,9 @@ virDomainMemoryDefCheckABIStability(virDomainMemoryDefPtr src, return false; } - if (memcmp(src->uuid, dst->uuid, VIR_UUID_BUFLEN) != 0) { + if ((src->uuid || dst->uuid) && + !(src->uuid && dst->uuid && + memcmp(src->uuid, dst->uuid, VIR_UUID_BUFLEN) == 0)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Target NVDIMM UUID doesn't match source NVDIMM")); return false; @@ -26560,7 +26564,6 @@ virDomainMemoryTargetDefFormat(virBufferPtr buf, static int virDomainMemoryDefFormat(virBufferPtr buf, virDomainMemoryDefPtr def, - const virDomainDef *dom, unsigned int flags) { const char *model = virDomainMemoryModelTypeToString(def->model); @@ -26575,8 +26578,7 @@ virDomainMemoryDefFormat(virBufferPtr buf, virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); - if (def->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM && - ARCH_IS_PPC64(dom->os.arch)) { + if (def->uuid) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(def->uuid, uuidstr); @@ -28974,7 +28976,7 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr def, virDomainShmemDefFormat(buf, def->shmems[n], flags); for (n = 0; n < def->nmems; n++) { - if (virDomainMemoryDefFormat(buf, def->mems[n], def, flags) < 0) + if (virDomainMemoryDefFormat(buf, def->mems[n], flags) < 0) return -1; } @@ -30091,7 +30093,7 @@ virDomainDeviceDefCopy(virDomainDeviceDefPtr src, rc = 0; break; case VIR_DOMAIN_DEVICE_MEMORY: - rc = virDomainMemoryDefFormat(&buf, src->data.memory, def, flags); + rc = virDomainMemoryDefFormat(&buf, src->data.memory, flags); break; case VIR_DOMAIN_DEVICE_SHMEM: virDomainShmemDefFormat(&buf, src->data.shmem, flags); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index f59d972c85..6645282bf6 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2331,7 +2331,7 @@ struct _virDomainMemoryDef { bool readonly; /* valid only for NVDIMM */ /* required for QEMU NVDIMM ppc64 support */ - unsigned char uuid[VIR_UUID_BUFLEN]; + unsigned char *uuid; virDomainDeviceInfo info; }; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 99761c217d..2506248866 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3319,7 +3319,7 @@ qemuBuildMemoryDeviceStr(const virDomainDef *def, if (mem->labelsize) virBufferAsprintf(&buf, "label-size=%llu,", mem->labelsize * 1024); - if (virUUIDIsValid(mem->uuid)) { + if (mem->uuid) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(mem->uuid, uuidstr); -- 2.26.2
On Mon, Jan 18, 2021 at 15:15:53 +0100, Michal Privoznik wrote:
The _virDomainMemoryDef structure has @uuid member which is needed for PPC64 guests. No other architectures use it. Since the member is VIR_UUID_BUFLEN bytes long, the structure is unnecessary big. If the member is just a pointer then we can also
Umm, this saves just 8 bytes. (pointer is 8, VIR_UUID_BUFLEN 16)
replace some calls of virUUIDIsValid() with plain test against NULL.
This commit isn't replacing them though (which is okay), but it also moves logic out of the XML formatter where we no longer need to check for the correct architecture, which isn't mentioned.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 14 ++++++++------ src/conf/domain_conf.h | 2 +- src/qemu/qemu_command.c | 2 +- 3 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ff3b7cbfc8..a2ddfcf947 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c
[...]
@@ -22808,7 +22810,9 @@ virDomainMemoryDefCheckABIStability(virDomainMemoryDefPtr src, return false; }
- if (memcmp(src->uuid, dst->uuid, VIR_UUID_BUFLEN) != 0) { + if ((src->uuid || dst->uuid) && + !(src->uuid && dst->uuid && + memcmp(src->uuid, dst->uuid, VIR_UUID_BUFLEN) == 0)) {
wrong alignment of the memcmp line since it's inside the second sub-term.
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Target NVDIMM UUID doesn't match source NVDIMM")); return false;
[...]
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index f59d972c85..6645282bf6 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2331,7 +2331,7 @@ struct _virDomainMemoryDef { bool readonly; /* valid only for NVDIMM */
/* required for QEMU NVDIMM ppc64 support */ - unsigned char uuid[VIR_UUID_BUFLEN]; + unsigned char *uuid;
Please add a comment that this is expected to be VIR_UUID_BUFLEN long buffer if allocated.
virDomainDeviceInfo info; };
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
It's better to fill in missing values in post parse callbacks than during parsing. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 35 ++++++++++++++++++++++------------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a2ddfcf947..4f0798de45 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5334,7 +5334,8 @@ virDomainVsockDefPostParse(virDomainVsockDefPtr vsock) static int -virDomainMemoryDefPostParse(virDomainMemoryDefPtr mem) +virDomainMemoryDefPostParse(virDomainMemoryDefPtr mem, + const virDomainDef *def) { switch (mem->model) { case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_PMEM: @@ -5345,6 +5346,19 @@ virDomainMemoryDefPostParse(virDomainMemoryDefPtr mem) break; case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: + /* If no NVDIMM UUID was provided in XML, generate one. */ + if (ARCH_IS_PPC64(def->os.arch) && + !mem->uuid) { + + mem->uuid = g_new0(unsigned char, VIR_UUID_BUFLEN); + if (virUUIDGenerate(mem->uuid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Failed to generate UUID")); + return -1; + } + } + break; + case VIR_DOMAIN_MEMORY_MODEL_DIMM: case VIR_DOMAIN_MEMORY_MODEL_NONE: case VIR_DOMAIN_MEMORY_MODEL_LAST: @@ -5401,7 +5415,7 @@ virDomainDeviceDefPostParseCommon(virDomainDeviceDefPtr dev, break; case VIR_DOMAIN_DEVICE_MEMORY: - ret = virDomainMemoryDefPostParse(dev->data.memory); + ret = virDomainMemoryDefPostParse(dev->data.memory, def); break; case VIR_DOMAIN_DEVICE_LEASE: @@ -15526,24 +15540,19 @@ virDomainMemoryDefParseXML(virDomainXMLOptionPtr xmlopt, } VIR_FREE(tmp); + /* Extract NVDIMM UUID. */ if (def->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM && - ARCH_IS_PPC64(dom->os.arch)) { - /* Extract nvdimm uuid or generate a new one */ - tmp = virXPathString("string(./uuid[1])", ctxt); - + ARCH_IS_PPC64(dom->os.arch) && + (tmp = virXPathString("string(./uuid[1])", ctxt))) { def->uuid = g_new0(unsigned char, VIR_UUID_BUFLEN); - if (!tmp) { - if (virUUIDGenerate(def->uuid) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Failed to generate UUID")); - goto error; - } - } else if (virUUIDParse(tmp, def->uuid) < 0) { + + if (virUUIDParse(tmp, def->uuid) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed uuid element")); goto error; } } + VIR_FREE(tmp); /* source */ if ((node = virXPathNode("./source", ctxt)) && -- 2.26.2
On Mon, Jan 18, 2021 at 15:15:54 +0100, Michal Privoznik wrote:
It's better to fill in missing values in post parse callbacks than during parsing.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 35 ++++++++++++++++++++++------------- 1 file changed, 22 insertions(+), 13 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a2ddfcf947..4f0798de45 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c
[...]
@@ -5345,6 +5346,19 @@ virDomainMemoryDefPostParse(virDomainMemoryDefPtr mem) break;
case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: + /* If no NVDIMM UUID was provided in XML, generate one. */ + if (ARCH_IS_PPC64(def->os.arch) && + !mem->uuid) { + + mem->uuid = g_new0(unsigned char, VIR_UUID_BUFLEN); + if (virUUIDGenerate(mem->uuid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Failed to generate UUID")); + return -1; + } + }
You can also reject if an UUID is present but the architecture isn't PPC64 here, rather than ...
+ break; + case VIR_DOMAIN_MEMORY_MODEL_DIMM: case VIR_DOMAIN_MEMORY_MODEL_NONE: case VIR_DOMAIN_MEMORY_MODEL_LAST:
[...]
@@ -15526,24 +15540,19 @@ virDomainMemoryDefParseXML(virDomainXMLOptionPtr xmlopt, } VIR_FREE(tmp);
+ /* Extract NVDIMM UUID. */ if (def->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM && - ARCH_IS_PPC64(dom->os.arch)) { - /* Extract nvdimm uuid or generate a new one */ - tmp = virXPathString("string(./uuid[1])", ctxt); - + ARCH_IS_PPC64(dom->os.arch) &&
... keeping it in the parser, where it doesn't make much sense.
+ (tmp = virXPathString("string(./uuid[1])", ctxt))) { def->uuid = g_new0(unsigned char, VIR_UUID_BUFLEN); - if (!tmp) { - if (virUUIDGenerate(def->uuid) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Failed to generate UUID")); - goto error; - } - } else if (virUUIDParse(tmp, def->uuid) < 0) { +
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
participants (2)
-
Michal Privoznik -
Peter Krempa