[PATCH v2 11/27] conf: Move some of virDomainMemoryDef members into a union
Peter Krempa
pkrempa at redhat.com
Fri Dec 4 07:58:29 UTC 2020
On Thu, Dec 03, 2020 at 13:36:14 +0100, Michal Privoznik wrote:
> The structure has two sets of members: some for the target and
> some for the source. The latter is model dependant (e.g. path to
> a nvdimm device applies only to NVDIMM model). Move the members
> into an union so that it is obvious which members apply to which
> model. This way it's easier to maintain code cleanliness when
> introducing a new model.
>
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
> src/conf/domain_conf.c | 57 ++++++++++++++++++------------
> src/conf/domain_conf.h | 16 ++++++---
> src/qemu/qemu_alias.c | 13 +++++--
> src/qemu/qemu_cgroup.c | 10 +++---
> src/qemu/qemu_command.c | 60 +++++++++++++++++++++-----------
> src/qemu/qemu_namespace.c | 2 +-
> src/qemu/qemu_process.c | 8 ++---
> src/security/security_apparmor.c | 6 ++--
> src/security/security_dac.c | 4 +--
> src/security/security_selinux.c | 4 +--
> src/security/virt-aa-helper.c | 2 +-
> 11 files changed, 113 insertions(+), 69 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 7d9e5d14ad..4d462b0627 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
[...]
> diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
> index dcb6c7156d..5ebcd766a9 100644
> --- a/src/qemu/qemu_alias.c
> +++ b/src/qemu/qemu_alias.c
> @@ -486,15 +486,22 @@ qemuAssignDeviceMemoryAlias(virDomainDefPtr def,
> size_t i;
> int maxidx = 0;
> int idx;
> - const char *prefix;
> + const char *prefix = NULL;
>
> if (mem->info.alias)
> return 0;
>
> - if (mem->model == VIR_DOMAIN_MEMORY_MODEL_DIMM)
> + switch (mem->model) {
> + case VIR_DOMAIN_MEMORY_MODEL_DIMM:
> prefix = "dimm";
> - else
> + break;
> + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM:
> prefix = "nvdimm";
> + break;
> + case VIR_DOMAIN_MEMORY_MODEL_NONE:
> + case VIR_DOMAIN_MEMORY_MODEL_LAST:
> + break;
> + }
Previously 'prefix' was guaranteed non-null in this function. This is no
longer true with this patch and 'prefix' will be used later without a
check. You need to add a 'default:' case and report enum error from the
switch to prevent that.
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index eb64ce84da..4bd45e0638 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -2958,10 +2958,13 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps,
> int rc;
> g_autoptr(virJSONValue) props = NULL;
> bool nodeSpecified = virDomainNumatuneNodeSpecified(def->numa, mem->targetNode);
> - unsigned long long pagesize = mem->pagesize;
> - bool needHugepage = !!pagesize;
> - bool useHugepage = !!pagesize;
> + unsigned long long pagesize = 0;
> + bool needHugepage = false;
> + bool useHugepage = false;
> int discard = mem->discard;
> + const char *nvdimmPath = NULL;
> + unsigned long long alignsize = 0;
> + bool nvdimmPmem = false;
>
> /* The difference between @needHugepage and @useHugepage is that the latter
> * is true whenever huge page is defined for the current memory cell.
> @@ -2971,6 +2974,23 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps,
>
> *backendProps = NULL;
>
> + switch (mem->model) {
> + case VIR_DOMAIN_MEMORY_MODEL_DIMM:
> + pagesize = mem->s.dimm.pagesize;
> + needHugepage = !!pagesize;
> + useHugepage = !!pagesize;
> + nodemask = mem->s.dimm.sourceNodes;
> + break;
> + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM:
> + nvdimmPath = mem->s.nvdimm.path;
> + alignsize = mem->s.nvdimm.alignsize;
> + nvdimmPmem = mem->s.nvdimm.pmem;
> + break;
> + case VIR_DOMAIN_MEMORY_MODEL_NONE:
> + case VIR_DOMAIN_MEMORY_MODEL_LAST:
> + break;
> + }
Add default/error as above.
Also this is not straight conversion to enum here. The scope of changes
was fairly limited in previous hunks, but here we are modifying multiple
variables and adding new ones. Peferrably, do such conversion prior to
the refactor next time.
> if (mem->targetNode >= 0) {
> /* memory devices could provide a invalid guest node */
> if (mem->targetNode >= virDomainNumaGetNodeCount(def->numa)) {
[...]
> @@ -3125,18 +3145,18 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps,
> if (virJSONValueObjectAdd(props, "U:size", mem->size * 1024, NULL) < 0)
> return -1;
>
> - if (mem->alignsize) {
> + if (alignsize) {
> if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE_ALIGN)) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("nvdimm align property is not available "
> "with this QEMU binary"));
> return -1;
> }
> - if (virJSONValueObjectAdd(props, "U:align", mem->alignsize * 1024, NULL) < 0)
> + if (virJSONValueObjectAdd(props, "U:align", alignsize * 1024, NULL) < 0)
> return -1;
> }
>
> - if (mem->nvdimmPmem) {
> + if (nvdimmPmem) {
This variable is read only once.
> if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE_PMEM)) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("nvdimm pmem property is not available "
> @@ -3147,13 +3167,10 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps,
> return -1;
> }
>
> - if (mem->sourceNodes) {
> - nodemask = mem->sourceNodes;
> - } else {
> - if (virDomainNumatuneMaybeGetNodeset(def->numa, priv->autoNodeset,
> - &nodemask, mem->targetNode) < 0)
> - return -1;
> - }
> + if (!nodemask &&
> + virDomainNumatuneMaybeGetNodeset(def->numa, priv->autoNodeset,
> + &nodemask, mem->targetNode) < 0)
> + return -1;
>
> if (nodemask) {
> if (!virNumaNodesetIsAvailable(nodemask))
> @@ -3166,8 +3183,11 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps,
> }
>
> /* If none of the following is requested... */
> - if (!needHugepage && !mem->sourceNodes && !nodeSpecified &&
> - !mem->nvdimmPath &&
> + if (!needHugepage &&
> + !(mem->model == VIR_DOMAIN_MEMORY_MODEL_DIMM &&
> + mem->s.dimm.sourceNodes) &&
> + !nodeSpecified &&
> + !nvdimmPath &&
> memAccess == VIR_DOMAIN_MEMORY_ACCESS_DEFAULT &&
> def->mem.source != VIR_DOMAIN_MEMORY_SOURCE_FILE &&
> def->mem.source != VIR_DOMAIN_MEMORY_SOURCE_MEMFD &&
Mixing of two refactors made this patch unnecessarily complex to review.
The patch itself is correct and I don't really want to see it again.
Thus you can commit it as-is provided that you follow up with a patch
which properly refactors qemuBuildMemoryBackendProps now that we have a
switch statement and aggregate code which is relevant for only one of
the memory models under the appropriate cases.
Reviewed-by: Peter Krempa <pkrempa at redhat.com>
More information about the libvir-list
mailing list