8:15 a.m.
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
8:15 a.m.
New subject: [PATCH 1/2] conf: Turn @uuid member of _virDomainMemoryDef struct into a pointer
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(a)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
8:38 a.m.
New subject: [PATCH 1/2] conf: Turn @uuid member of _virDomainMemoryDef struct into a pointer
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(a)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(a)redhat.com>
8:15 a.m.
New subject: [PATCH 2/2] conf: Move generation of NVDIMM UUID into post parse callback
It's better to fill in missing values in post parse callbacks
than during parsing.
Signed-off-by: Michal Privoznik <mprivozn(a)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
8:40 a.m.
New subject: [PATCH 2/2] conf: Move generation of NVDIMM UUID into post parse callback
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(a)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(a)redhat.com>
1371
days inactive
1371
days old
4 comments
2 participants
participants (2)
-
Michal Privoznik -
Peter Krempa