[PATCH 00/21] move checks from parse functions to post parse
Hi, This started as a simple NVDIMM change, then I realized there is a Gitlab work item for it [1], so I took the extra mile and did a bit more. I'll copy/paste here the motivation for this kind of change, provided by Cole in [1]: ----- The code that handles domain/VM XML parsing (virDomainDefParseXML in src/domain/domain_conf.c) has various validation checks sprinkled within it. Many of these checks should be moved out of the parse step and into virDomainDefPostParse, so they can be shared by various places in the code that build a domain definition without XML, such as converting from native vmware/virtualbox/xen formats. ----- There are still checks to be moved after this work. If this is accepted I'll update [1] with more details/tips about how to proceed with the remaining checks. Some g_auto* cleanups were done along the way. [1] https://gitlab.com/libvirt/libvirt/-/issues/7 Daniel Henrique Barboza (21): domain_conf.c: move NVDIMM 'labelsize' check to post parse domain_conf.c: use g_autofree in 'dev' in virDomainDefParseBootXML() domain_conf.c: modernize virDomainDefBootOrderPostParse() domain_conf.c: move boot related timeouts check to post parse domain_conf.c: do not leak 'video' in virDomainDefParseXML() domain_conf.c: move primary video check to virDomainDefPostParseVideo() domain_conf.c: use g_autoptr() with virDomainVideoDefPtr domain_conf.c: move QXL attributes check to virDomainVideoDefPostParse() virstorageencryption.h: add AUTOPTR_CLEANUP_FUNC for virStorageEncryptionPtr domain_conf: modernize virDomainDiskDefParseXML() domain_conf.c: move vendor, product and tray checks to post parse domain_conf.c: move smartcard address check to post parse domain_conf.c: modernize virDomainSmartcardDefParseXML domain_conf.c: remove 'error' label in virDomainDefTunablesParse() domain_conf.c: move duplicate blkio path check to post parse domain_conf.c: move virDomainPCIControllerOpts checks to post parse domain_conf.c: move pci-root/pcie-root address check to post parse domain_conf.c: modernize virDomainControllerDefParseXML() domain_conf.c: modernize virDomainDefControllersParse() domain_conf.c: use VIR_ERR_CONFIG_UNSUPPORTED in post parse domain_conf.c: move idmapEntry checks to post parse src/conf/domain_conf.c | 762 ++++++++++-------- src/conf/domain_conf.h | 4 + src/util/virstorageencryption.h | 1 + tests/qemuxml2argvdata/pci-root-address.err | 2 +- .../pseries-default-phb-numa-node.err | 2 +- .../video-multiple-primaries.err | 1 + .../video-multiple-primaries.xml | 32 + tests/qemuxml2argvtest.c | 5 + 8 files changed, 451 insertions(+), 358 deletions(-) create mode 100644 tests/qemuxml2argvdata/video-multiple-primaries.err create mode 100644 tests/qemuxml2argvdata/video-multiple-primaries.xml -- 2.26.2
Move 'labelsize' validation to virDomainMemoryDefPostParse(). Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 43 +++++++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 21 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b1534dcc1e..5e5905f483 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5363,15 +5363,28 @@ static int virDomainMemoryDefPostParse(virDomainMemoryDefPtr mem, const virDomainDef *def) { - /* Although only the QEMU driver implements PPC64 support, this - * code is related to the platform specification (PAPR), i.e. it - * is hypervisor agnostic, and any future PPC64 hypervisor driver - * will have the same restriction. - */ - if (ARCH_IS_PPC64(def->os.arch) && - mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM && - virDomainNVDimmAlignSizePseries(mem) < 0) - return -1; + if (mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) { + if (mem->labelsize && mem->labelsize < 128) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("nvdimm label must be at least 128KiB")); + return -1; + } + + if (mem->labelsize >= mem->size) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("label size must be smaller than NVDIMM size")); + return -1; + } + + /* Although only the QEMU driver implements PPC64 support, this + * code is related to the platform specification (PAPR), i.e. it + * is hypervisor agnostic, and any future PPC64 hypervisor driver + * will have the same restriction. + */ + if (ARCH_IS_PPC64(def->os.arch) && + virDomainNVDimmAlignSizePseries(mem) < 0) + return -1; + } return 0; } @@ -16766,18 +16779,6 @@ virDomainMemoryTargetDefParseXML(xmlNodePtr node, &def->labelsize, false, false) < 0) return -1; - if (def->labelsize && def->labelsize < 128) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("nvdimm label must be at least 128KiB")); - return -1; - } - - if (def->labelsize >= def->size) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("label size must be smaller than NVDIMM size")); - return -1; - } - if (virXPathBoolean("boolean(./readonly)", ctxt)) def->readonly = true; } -- 2.26.2
On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote:
Move 'labelsize' validation to virDomainMemoryDefPostParse().
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 43 +++++++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 21 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b1534dcc1e..5e5905f483 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5363,15 +5363,28 @@ static int virDomainMemoryDefPostParse(virDomainMemoryDefPtr mem, const virDomainDef *def) { - /* Although only the QEMU driver implements PPC64 support, this - * code is related to the platform specification (PAPR), i.e. it - * is hypervisor agnostic, and any future PPC64 hypervisor driver - * will have the same restriction. - */ - if (ARCH_IS_PPC64(def->os.arch) && - mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM && - virDomainNVDimmAlignSizePseries(mem) < 0) - return -1; + if (mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) { + if (mem->labelsize && mem->labelsize < 128) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("nvdimm label must be at least 128KiB")); + return -1; + } + + if (mem->labelsize >= mem->size) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("label size must be smaller than NVDIMM size")); + return -1; + } + + /* Although only the QEMU driver implements PPC64 support, this + * code is related to the platform specification (PAPR), i.e. it + * is hypervisor agnostic, and any future PPC64 hypervisor driver + * will have the same restriction. + */ + if (ARCH_IS_PPC64(def->os.arch) && + virDomainNVDimmAlignSizePseries(mem) < 0) + return -1; + }
For this and the rest of patches - shouldn't changes like this go into validator callback? I view post parse callbacks as "fill missing values" not a place to check if configuration makes sense/is valid. Michal
On 12/1/20 3:46 PM, Michal Privoznik wrote:
On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote:
Move 'labelsize' validation to virDomainMemoryDefPostParse().
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 43 +++++++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 21 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b1534dcc1e..5e5905f483 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5363,15 +5363,28 @@ static int virDomainMemoryDefPostParse(virDomainMemoryDefPtr mem, const virDomainDef *def) { - /* Although only the QEMU driver implements PPC64 support, this - * code is related to the platform specification (PAPR), i.e. it - * is hypervisor agnostic, and any future PPC64 hypervisor driver - * will have the same restriction. - */ - if (ARCH_IS_PPC64(def->os.arch) && - mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM && - virDomainNVDimmAlignSizePseries(mem) < 0) - return -1; + if (mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) { + if (mem->labelsize && mem->labelsize < 128) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("nvdimm label must be at least 128KiB")); + return -1; + } + + if (mem->labelsize >= mem->size) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("label size must be smaller than NVDIMM size")); + return -1; + } + + /* Although only the QEMU driver implements PPC64 support, this + * code is related to the platform specification (PAPR), i.e. it + * is hypervisor agnostic, and any future PPC64 hypervisor driver + * will have the same restriction. + */ + if (ARCH_IS_PPC64(def->os.arch) && + virDomainNVDimmAlignSizePseries(mem) < 0) + return -1; + }
For this and the rest of patches - shouldn't changes like this go into validator callback? I view post parse callbacks as "fill missing values" not a place to check if configuration makes sense/is valid.
You mean these callbacks? ---- domain_conf.h ---- /* validation callbacks */ virDomainDefValidateCallback domainValidateCallback; virDomainDeviceDefValidateCallback deviceValidateCallback; These callbacks makes sense for driver specific validations, but for what we're doing here is driver agnostic (well, most of it anyway - probably there are QEMU specific stuff mixed in). If we move this logic to say qemuValidateDomainDeviceDef(), then we'll need to compensate the other drivers that won't have access to these validations (git grep tells me it's bhyve and vz_driver). Granted, we can put these in an unique function and use them in the callback for all the drivers, if that's the case. Thanks, DHB
Michal
On 12/1/20 8:58 PM, Daniel Henrique Barboza wrote:
On 12/1/20 3:46 PM, Michal Privoznik wrote:
On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote:
Move 'labelsize' validation to virDomainMemoryDefPostParse().
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 43 +++++++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 21 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b1534dcc1e..5e5905f483 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5363,15 +5363,28 @@ static int virDomainMemoryDefPostParse(virDomainMemoryDefPtr mem, const virDomainDef *def) { - /* Although only the QEMU driver implements PPC64 support, this - * code is related to the platform specification (PAPR), i.e. it - * is hypervisor agnostic, and any future PPC64 hypervisor driver - * will have the same restriction. - */ - if (ARCH_IS_PPC64(def->os.arch) && - mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM && - virDomainNVDimmAlignSizePseries(mem) < 0) - return -1; + if (mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) { + if (mem->labelsize && mem->labelsize < 128) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("nvdimm label must be at least 128KiB")); + return -1; + } + + if (mem->labelsize >= mem->size) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("label size must be smaller than NVDIMM size")); + return -1; + } + + /* Although only the QEMU driver implements PPC64 support, this + * code is related to the platform specification (PAPR), i.e. it + * is hypervisor agnostic, and any future PPC64 hypervisor driver + * will have the same restriction. + */ + if (ARCH_IS_PPC64(def->os.arch) && + virDomainNVDimmAlignSizePseries(mem) < 0) + return -1; + }
For this and the rest of patches - shouldn't changes like this go into validator callback? I view post parse callbacks as "fill missing values" not a place to check if configuration makes sense/is valid.
You mean these callbacks?
---- domain_conf.h ----
/* validation callbacks */ virDomainDefValidateCallback domainValidateCallback; virDomainDeviceDefValidateCallback deviceValidateCallback;
I mean virDomainDefValidate() and more specifically virDomainDefValidateInternal(). Driver specific callbacks are out of question - exactly for the reason you pointed out. Michal
On 12/1/20 5:20 PM, Michal Privoznik wrote:
On 12/1/20 8:58 PM, Daniel Henrique Barboza wrote:
On 12/1/20 3:46 PM, Michal Privoznik wrote:
On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote:
Move 'labelsize' validation to virDomainMemoryDefPostParse().
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 43 +++++++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 21 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b1534dcc1e..5e5905f483 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5363,15 +5363,28 @@ static int virDomainMemoryDefPostParse(virDomainMemoryDefPtr mem, const virDomainDef *def) { - /* Although only the QEMU driver implements PPC64 support, this - * code is related to the platform specification (PAPR), i.e. it - * is hypervisor agnostic, and any future PPC64 hypervisor driver - * will have the same restriction. - */ - if (ARCH_IS_PPC64(def->os.arch) && - mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM && - virDomainNVDimmAlignSizePseries(mem) < 0) - return -1; + if (mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) { + if (mem->labelsize && mem->labelsize < 128) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("nvdimm label must be at least 128KiB")); + return -1; + } + + if (mem->labelsize >= mem->size) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("label size must be smaller than NVDIMM size")); + return -1; + } + + /* Although only the QEMU driver implements PPC64 support, this + * code is related to the platform specification (PAPR), i.e. it + * is hypervisor agnostic, and any future PPC64 hypervisor driver + * will have the same restriction. + */ + if (ARCH_IS_PPC64(def->os.arch) && + virDomainNVDimmAlignSizePseries(mem) < 0) + return -1; + }
For this and the rest of patches - shouldn't changes like this go into validator callback? I view post parse callbacks as "fill missing values" not a place to check if configuration makes sense/is valid.
You mean these callbacks?
---- domain_conf.h ----
/* validation callbacks */ virDomainDefValidateCallback domainValidateCallback; virDomainDeviceDefValidateCallback deviceValidateCallback;
I mean virDomainDefValidate() and more specifically virDomainDefValidateInternal(). Driver specific callbacks are out of question - exactly for the reason you pointed out.
Got it. I'll not overload the PostParse() functions and, instead, use virDomainDefValidateInternal() and virDomainDeviceDefValidateInternal() for these cases. Let's try it again in v2. Thanks, DHB
Michal
On 12/1/20 10:03 PM, Daniel Henrique Barboza wrote:
On 12/1/20 5:20 PM, Michal Privoznik wrote:
On 12/1/20 8:58 PM, Daniel Henrique Barboza wrote:
On 12/1/20 3:46 PM, Michal Privoznik wrote:
On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote:
Move 'labelsize' validation to virDomainMemoryDefPostParse().
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 43 +++++++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 21 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b1534dcc1e..5e5905f483 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5363,15 +5363,28 @@ static int virDomainMemoryDefPostParse(virDomainMemoryDefPtr mem, const virDomainDef *def) { - /* Although only the QEMU driver implements PPC64 support, this - * code is related to the platform specification (PAPR), i.e. it - * is hypervisor agnostic, and any future PPC64 hypervisor driver - * will have the same restriction. - */ - if (ARCH_IS_PPC64(def->os.arch) && - mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM && - virDomainNVDimmAlignSizePseries(mem) < 0) - return -1; + if (mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) { + if (mem->labelsize && mem->labelsize < 128) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("nvdimm label must be at least 128KiB")); + return -1; + } + + if (mem->labelsize >= mem->size) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("label size must be smaller than NVDIMM size")); + return -1; + } + + /* Although only the QEMU driver implements PPC64 support, this + * code is related to the platform specification (PAPR), i.e. it + * is hypervisor agnostic, and any future PPC64 hypervisor driver + * will have the same restriction. + */ + if (ARCH_IS_PPC64(def->os.arch) && + virDomainNVDimmAlignSizePseries(mem) < 0) + return -1; + }
For this and the rest of patches - shouldn't changes like this go into validator callback? I view post parse callbacks as "fill missing values" not a place to check if configuration makes sense/is valid.
You mean these callbacks?
---- domain_conf.h ----
/* validation callbacks */ virDomainDefValidateCallback domainValidateCallback; virDomainDeviceDefValidateCallback deviceValidateCallback;
I mean virDomainDefValidate() and more specifically virDomainDefValidateInternal(). Driver specific callbacks are out of question - exactly for the reason you pointed out.
Got it. I'll not overload the PostParse() functions and, instead, use virDomainDefValidateInternal() and virDomainDeviceDefValidateInternal() for these cases.
Let's try it again in v2.
Yeah, you can merge those cleanup patches to which I replied with my reviewed-by. I vaguely recall that I might merge some patches of your that did something similar - moved checks from parser to post parse, do you remember? If so, I'm sorry that I misled you. Michal
On 12/1/20 6:16 PM, Michal Privoznik wrote:
On 12/1/20 10:03 PM, Daniel Henrique Barboza wrote:
On 12/1/20 5:20 PM, Michal Privoznik wrote:
On 12/1/20 8:58 PM, Daniel Henrique Barboza wrote:
On 12/1/20 3:46 PM, Michal Privoznik wrote:
On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote:
Move 'labelsize' validation to virDomainMemoryDefPostParse().
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 43 +++++++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 21 deletions(-)
[...]
+ /* Although only the QEMU driver implements PPC64 support, this + * code is related to the platform specification (PAPR), i.e. it + * is hypervisor agnostic, and any future PPC64 hypervisor driver + * will have the same restriction. + */ + if (ARCH_IS_PPC64(def->os.arch) && + virDomainNVDimmAlignSizePseries(mem) < 0) + return -1; + }
For this and the rest of patches - shouldn't changes like this go into validator callback? I view post parse callbacks as "fill missing values" not a place to check if configuration makes sense/is valid.
You mean these callbacks?
---- domain_conf.h ----
/* validation callbacks */ virDomainDefValidateCallback domainValidateCallback; virDomainDeviceDefValidateCallback deviceValidateCallback;
I mean virDomainDefValidate() and more specifically virDomainDefValidateInternal(). Driver specific callbacks are out of question - exactly for the reason you pointed out.
Got it. I'll not overload the PostParse() functions and, instead, use virDomainDefValidateInternal() and virDomainDeviceDefValidateInternal() for these cases.
Let's try it again in v2.
Yeah, you can merge those cleanup patches to which I replied with my reviewed-by.
Just did. Thanks for the reviews!
I vaguely recall that I might merge some patches of your that did something similar - moved checks from parser to post parse, do you remember? If so, I'm sorry that I misled you.
Nah don't worry about it. It's all learning experience. Besides, the only one instance I'm recalling doing that ATM is with a NVDIMM code that wasn't you who merged. (and this particular code can be moved to the proper place like we discussed above. I'll keep that in mind when sending the v2). Thanks, DHB
Michal
This spares us of 2 explicit VIR_FREE() calls. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5e5905f483..4229add26d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18908,7 +18908,7 @@ virDomainDefParseBootXML(xmlXPathContextPtr ctxt, for (i = 0; i < n && i < VIR_DOMAIN_BOOT_LAST; i++) { int val; - char *dev = virXMLPropString(nodes[i], "dev"); + g_autofree char *dev = virXMLPropString(nodes[i], "dev"); if (!dev) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing boot device")); @@ -18918,10 +18918,8 @@ virDomainDefParseBootXML(xmlXPathContextPtr ctxt, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown boot device '%s'"), dev); - VIR_FREE(dev); return -1; } - VIR_FREE(dev); def->os.bootDevs[def->os.nBootDevs++] = val; } -- 2.26.2
On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote:
This spares us of 2 explicit VIR_FREE() calls.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
Use g_autoptr() with the hash and remove the 'cleanup' label. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4229add26d..f0300b870d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5823,20 +5823,19 @@ virDomainDefCollectBootOrder(virDomainDefPtr def G_GNUC_UNUSED, static int virDomainDefBootOrderPostParse(virDomainDefPtr def) { - GHashTable *bootHash = NULL; - int ret = -1; + g_autoptr(GHashTable) bootHash = NULL; if (!(bootHash = virHashNew(NULL))) - goto cleanup; + return -1; if (virDomainDeviceInfoIterate(def, virDomainDefCollectBootOrder, bootHash) < 0) - goto cleanup; + return -1; if (def->os.nBootDevs > 0 && virHashSize(bootHash) > 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("per-device boot elements cannot be used" " together with os/boot elements")); - goto cleanup; + return -1; } if (def->os.nBootDevs == 0 && virHashSize(bootHash) == 0) { @@ -5844,11 +5843,7 @@ virDomainDefBootOrderPostParse(virDomainDefPtr def) def->os.bootDevs[0] = VIR_DOMAIN_BOOT_DISK; } - ret = 0; - - cleanup: - virHashFree(bootHash); - return ret; + return 0; } -- 2.26.2
On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote:
Use g_autoptr() with the hash and remove the 'cleanup' label.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
This patch creates a new function, virDomainDefBootPostParse(), to host the validation of boot menu timeout and rebootTimeout to post parse time. The checks in virDomainDefParseBootXML() were changed to throw VIR_ERR_XML_ERROR in case of parse error of those values. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 41 +++++++++++++++++++++++++++++++---------- 1 file changed, 31 insertions(+), 10 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f0300b870d..c2e5ed5680 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5847,6 +5847,28 @@ virDomainDefBootOrderPostParse(virDomainDefPtr def) } +static int +virDomainDefBootPostParse(virDomainDefPtr def) +{ + if (def->os.bm_timeout_set && def->os.bm_timeout > 65535) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("invalid value for boot menu timeout, " + "must be in range [0,65535]")); + return -1; + } + + if (def->os.bios.rt_set && + (def->os.bios.rt_delay < -1 || def->os.bios.rt_delay > 65535)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("invalid value for rebootTimeout, " + "must be in range [-1,65535]")); + return -1; + } + + return 0; +} + + static int virDomainDefPostParseVideo(virDomainDefPtr def, void *opaque) @@ -5917,6 +5939,9 @@ virDomainDefPostParseCommon(virDomainDefPtr def, if (virDomainDefRejectDuplicatePanics(def) < 0) return -1; + if (virDomainDefBootPostParse(def) < 0) + return -1; + if (def->os.type == VIR_DOMAIN_OSTYPE_HVM && !(data->xmlopt->config.features & VIR_DOMAIN_DEF_FEATURE_NO_BOOT_ORDER) && virDomainDefBootOrderPostParse(def) < 0) @@ -18935,11 +18960,9 @@ virDomainDefParseBootXML(xmlXPathContextPtr ctxt, tmp = virXMLPropString(node, "timeout"); if (tmp && def->os.bootmenu == VIR_TRISTATE_BOOL_YES) { - if (virStrToLong_uip(tmp, NULL, 0, &def->os.bm_timeout) < 0 || - def->os.bm_timeout > 65535) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("invalid value for boot menu timeout, " - "must be in range [0,65535]")); + if (virStrToLong_uip(tmp, NULL, 0, &def->os.bm_timeout) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("invalid value for boot menu timeout")); return -1; } def->os.bm_timeout_set = true; @@ -18960,11 +18983,9 @@ virDomainDefParseBootXML(xmlXPathContextPtr ctxt, if (tmp) { /* that was really just for the check if it is there */ - if (virStrToLong_i(tmp, NULL, 0, &def->os.bios.rt_delay) < 0 || - def->os.bios.rt_delay < -1 || def->os.bios.rt_delay > 65535) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("invalid value for rebootTimeout, " - "must be in range [-1,65535]")); + if (virStrToLong_i(tmp, NULL, 0, &def->os.bios.rt_delay) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("invalid value for rebootTimeout")); return -1; } def->os.bios.rt_set = true; -- 2.26.2
The 'video' pointer is only being freed on error path, meaning that we're leaking it after each loop restart. There are more opportunities for auto cleanups of virDomainVideoDef pointers, so let's register AUTOPTR_CLEANUP_FUNC for it to use g_autoptr() later on. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 4 +--- src/conf/domain_conf.h | 1 + 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c2e5ed5680..88c647f943 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -22233,7 +22233,7 @@ virDomainDefParseXML(xmlDocPtr xml, if (n) def->videos = g_new0(virDomainVideoDefPtr, n); for (i = 0; i < n; i++) { - virDomainVideoDefPtr video; + g_autoptr(virDomainVideoDef) video = NULL; ssize_t insertAt = -1; if (!(video = virDomainVideoDefParseXML(xmlopt, nodes[i], @@ -22242,7 +22242,6 @@ virDomainDefParseXML(xmlDocPtr xml, if (video->primary) { if (def->nvideos != 0 && def->videos[0]->primary) { - virDomainVideoDefFree(video); virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Only one primary video device is supported")); goto error; @@ -22254,7 +22253,6 @@ virDomainDefParseXML(xmlDocPtr xml, insertAt, def->nvideos, video) < 0) { - virDomainVideoDefFree(video); goto error; } } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 96e6c34553..ff82da7f7e 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3083,6 +3083,7 @@ void virDomainNVRAMDefFree(virDomainNVRAMDefPtr def); void virDomainWatchdogDefFree(virDomainWatchdogDefPtr def); virDomainVideoDefPtr virDomainVideoDefNew(virDomainXMLOptionPtr xmlopt); void virDomainVideoDefFree(virDomainVideoDefPtr def); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainVideoDef, virDomainVideoDefFree); void virDomainVideoDefClear(virDomainVideoDefPtr def); virDomainHostdevDefPtr virDomainHostdevDefNew(void); void virDomainHostdevDefClear(virDomainHostdevDefPtr def); -- 2.26.2
On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote:
The 'video' pointer is only being freed on error path, meaning that we're leaking it after each loop restart.
There are more opportunities for auto cleanups of virDomainVideoDef pointers, so let's register AUTOPTR_CLEANUP_FUNC for it to use g_autoptr() later on.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 4 +--- src/conf/domain_conf.h | 1 + 2 files changed, 2 insertions(+), 3 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
Move it to post parse since it's not related to XML parsing. Since we don't have a failure test for this scenario, a new 'video-multiple-primaries' test was added to test this failure case. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 24 +++++++++----- .../video-multiple-primaries.err | 1 + .../video-multiple-primaries.xml | 32 +++++++++++++++++++ tests/qemuxml2argvtest.c | 5 +++ 4 files changed, 54 insertions(+), 8 deletions(-) create mode 100644 tests/qemuxml2argvdata/video-multiple-primaries.err create mode 100644 tests/qemuxml2argvdata/video-multiple-primaries.xml diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 88c647f943..521a7cee3a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5873,9 +5873,23 @@ static int virDomainDefPostParseVideo(virDomainDefPtr def, void *opaque) { + size_t i; + if (def->nvideos == 0) return 0; + /* Any video marked as primary will be put in index 0 by the + * parser. Ensure that we have only one primary set by the user. */ + if (def->videos[0]->primary) { + for (i = 1; i < def->nvideos; i++) { + if (def->videos[i]->primary) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Only one primary video device is supported")); + return -1; + } + } + } + if (def->videos[0]->type == VIR_DOMAIN_VIDEO_TYPE_NONE) { char *alias; @@ -22240,15 +22254,9 @@ virDomainDefParseXML(xmlDocPtr xml, ctxt, flags))) goto error; - if (video->primary) { - if (def->nvideos != 0 && def->videos[0]->primary) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Only one primary video device is supported")); - goto error; - } - + if (video->primary) insertAt = 0; - } + if (VIR_INSERT_ELEMENT_INPLACE(def->videos, insertAt, def->nvideos, diff --git a/tests/qemuxml2argvdata/video-multiple-primaries.err b/tests/qemuxml2argvdata/video-multiple-primaries.err new file mode 100644 index 0000000000..f81b7275e4 --- /dev/null +++ b/tests/qemuxml2argvdata/video-multiple-primaries.err @@ -0,0 +1 @@ +unsupported configuration: Only one primary video device is supported diff --git a/tests/qemuxml2argvdata/video-multiple-primaries.xml b/tests/qemuxml2argvdata/video-multiple-primaries.xml new file mode 100644 index 0000000000..977227c5ff --- /dev/null +++ b/tests/qemuxml2argvdata/video-multiple-primaries.xml @@ -0,0 +1,32 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-i386</emulator> + <disk type='file' device='disk'> + <driver name='qemu' type='qcow2' cache='none'/> + <source file='/var/lib/libvirt/images/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <video> + <model type='vga' vram='16384' heads='1' primary='yes'/> + </video> + <video> + <model type='qxl' heads='1' primary='yes'/> + </video> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 42d147243e..9043c9bdbe 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2378,6 +2378,11 @@ mymain(void) DO_TEST_CAPS_ARCH_LATEST("default-video-type-riscv64", "riscv64"); DO_TEST_CAPS_ARCH_LATEST("default-video-type-s390x", "s390x"); + DO_TEST_PARSE_ERROR("video-multiple-primaries", + QEMU_CAPS_DEVICE_QXL, + QEMU_CAPS_DEVICE_VGA, + QEMU_CAPS_DEVICE_VIDEO_PRIMARY); + DO_TEST("virtio-rng-default", QEMU_CAPS_DEVICE_VIRTIO_RNG, QEMU_CAPS_OBJECT_RNG_RANDOM); -- 2.26.2
This will modernize virDomainVideoDefParseXML() and virDomainDefAddImplicitVideo() by removing unneeded cleanup labels. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 48 ++++++++++++++++++------------------------ 1 file changed, 20 insertions(+), 28 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 521a7cee3a..8352c60473 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16179,7 +16179,7 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt, xmlXPathContextPtr ctxt, unsigned int flags) { - virDomainVideoDefPtr def; + g_autoptr(virDomainVideoDef) def = NULL; xmlNodePtr cur; VIR_XPATH_NODE_AUTORESTORE(ctxt) g_autofree char *type = NULL; @@ -16220,12 +16220,12 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt, if (def->accel == NULL && virXMLNodeNameEqual(child, "acceleration")) { if ((def->accel = virDomainVideoAccelDefParseXML(child)) == NULL) - goto error; + return NULL; } if (def->res == NULL && virXMLNodeNameEqual(child, "resolution")) { if ((def->res = virDomainVideoResolutionDefParseXML(child)) == NULL) - goto error; + return NULL; } } child = child->next; @@ -16233,7 +16233,7 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt, } if (virXMLNodeNameEqual(cur, "driver")) { if (virDomainVirtioOptionsParseXML(cur, &def->virtio) < 0) - goto error; + return NULL; driver_name = virXMLPropString(cur, "name"); } } @@ -16244,7 +16244,7 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt, if ((def->type = virDomainVideoTypeFromString(type)) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown video model '%s'"), type); - goto error; + return NULL; } } else { def->type = VIR_DOMAIN_VIDEO_TYPE_DEFAULT; @@ -16255,7 +16255,7 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt, if ((backend = virDomainVideoBackendTypeFromString(driver_name)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown video driver '%s'"), driver_name); - goto error; + return NULL; } def->backend = backend; } else { @@ -16266,12 +16266,12 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt, if (def->type != VIR_DOMAIN_VIDEO_TYPE_QXL) { virReportError(VIR_ERR_XML_ERROR, "%s", _("ram attribute only supported for type of qxl")); - goto error; + return NULL; } if (virStrToLong_uip(ram, NULL, 10, &def->ram) < 0) { virReportError(VIR_ERR_XML_ERROR, _("cannot parse video ram '%s'"), ram); - goto error; + return NULL; } } @@ -16279,7 +16279,7 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt, if (virStrToLong_uip(vram, NULL, 10, &def->vram) < 0) { virReportError(VIR_ERR_XML_ERROR, _("cannot parse video vram '%s'"), vram); - goto error; + return NULL; } } @@ -16287,12 +16287,12 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt, if (def->type != VIR_DOMAIN_VIDEO_TYPE_QXL) { virReportError(VIR_ERR_XML_ERROR, "%s", _("vram64 attribute only supported for type of qxl")); - goto error; + return NULL; } if (virStrToLong_uip(vram64, NULL, 10, &def->vram64) < 0) { virReportError(VIR_ERR_XML_ERROR, _("cannot parse video vram64 '%s'"), vram64); - goto error; + return NULL; } } @@ -16300,12 +16300,12 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt, if (def->type != VIR_DOMAIN_VIDEO_TYPE_QXL) { virReportError(VIR_ERR_XML_ERROR, "%s", _("vgamem attribute only supported for type of qxl")); - goto error; + return NULL; } if (virStrToLong_uip(vgamem, NULL, 10, &def->vgamem) < 0) { virReportError(VIR_ERR_XML_ERROR, _("cannot parse video vgamem '%s'"), vgamem); - goto error; + return NULL; } } @@ -16313,20 +16313,16 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt, if (virStrToLong_uip(heads, NULL, 10, &def->heads) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot parse video heads '%s'"), heads); - goto error; + return NULL; } } if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, flags) < 0) - goto error; + return NULL; def->driver = virDomainVideoDriverDefParseXML(node); - return def; - - error: - virDomainVideoDefFree(def); - return NULL; + return g_steal_pointer(&def); } static virDomainHostdevDefPtr @@ -25038,8 +25034,7 @@ virDomainDefAddImplicitControllers(virDomainDefPtr def) static int virDomainDefAddImplicitVideo(virDomainDefPtr def, virDomainXMLOptionPtr xmlopt) { - int ret = -1; - virDomainVideoDefPtr video = NULL; + g_autoptr(virDomainVideoDef) video = NULL; /* For backwards compatibility, if no <video> tag is set but there * is a <graphics> tag, then we add a single video tag */ @@ -25047,15 +25042,12 @@ virDomainDefAddImplicitVideo(virDomainDefPtr def, virDomainXMLOptionPtr xmlopt) return 0; if (!(video = virDomainVideoDefNew(xmlopt))) - goto cleanup; + return -1; video->type = VIR_DOMAIN_VIDEO_TYPE_DEFAULT; if (VIR_APPEND_ELEMENT(def->videos, def->nvideos, video) < 0) - goto cleanup; + return -1; - ret = 0; - cleanup: - virDomainVideoDefFree(video); - return ret; + return 0; } int -- 2.26.2
On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote:
This will modernize virDomainVideoDefParseXML() and virDomainDefAddImplicitVideo() by removing unneeded cleanup labels.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 48 ++++++++++++++++++------------------------ 1 file changed, 20 insertions(+), 28 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
These checks are not related to XML parsing and can be moved to post parse time. Errors were changed from VIR_ERR_XML_ERROR to VIR_ERR_CONFIG_UNSUPPORTED. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 43 ++++++++++++++++++++++++------------------ 1 file changed, 25 insertions(+), 18 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8352c60473..a8d82f4733 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5303,10 +5303,31 @@ virDomainDiskDefPostParse(virDomainDiskDefPtr disk, } -static void +static int virDomainVideoDefPostParse(virDomainVideoDefPtr video, const virDomainDef *def) { + + if (video->type != VIR_DOMAIN_VIDEO_TYPE_QXL) { + if (video->ram != 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("ram attribute only supported for video type qxl")); + return -1; + } + + if (video->vram64 != 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("vram64 attribute only supported for video type qxl")); + return -1; + } + + if (video->vgamem != 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("vgamem attribute only supported for video type qxl")); + return -1; + } + } + /* Fill out (V)RAM if the driver-specific callback did not do so */ if (video->ram == 0 && video->type == VIR_DOMAIN_VIDEO_TYPE_QXL) video->ram = virDomainVideoDefaultRAM(def, video->type); @@ -5315,6 +5336,8 @@ virDomainVideoDefPostParse(virDomainVideoDefPtr video, video->ram = VIR_ROUND_UP_POWER_OF_TWO(video->ram); video->vram = VIR_ROUND_UP_POWER_OF_TWO(video->vram); + + return 0; } @@ -5414,8 +5437,7 @@ virDomainDeviceDefPostParseCommon(virDomainDeviceDefPtr dev, break; case VIR_DOMAIN_DEVICE_VIDEO: - virDomainVideoDefPostParse(dev->data.video, def); - ret = 0; + ret = virDomainVideoDefPostParse(dev->data.video, def); break; case VIR_DOMAIN_DEVICE_HOSTDEV: @@ -16263,11 +16285,6 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt, } if (ram) { - if (def->type != VIR_DOMAIN_VIDEO_TYPE_QXL) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("ram attribute only supported for type of qxl")); - return NULL; - } if (virStrToLong_uip(ram, NULL, 10, &def->ram) < 0) { virReportError(VIR_ERR_XML_ERROR, _("cannot parse video ram '%s'"), ram); @@ -16284,11 +16301,6 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt, } if (vram64) { - if (def->type != VIR_DOMAIN_VIDEO_TYPE_QXL) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("vram64 attribute only supported for type of qxl")); - return NULL; - } if (virStrToLong_uip(vram64, NULL, 10, &def->vram64) < 0) { virReportError(VIR_ERR_XML_ERROR, _("cannot parse video vram64 '%s'"), vram64); @@ -16297,11 +16309,6 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt, } if (vgamem) { - if (def->type != VIR_DOMAIN_VIDEO_TYPE_QXL) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("vgamem attribute only supported for type of qxl")); - return NULL; - } if (virStrToLong_uip(vgamem, NULL, 10, &def->vgamem) < 0) { virReportError(VIR_ERR_XML_ERROR, _("cannot parse video vgamem '%s'"), vgamem); -- 2.26.2
This will open an opportunity to modernize virDomainDiskDefParseXML() in the next patch. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/util/virstorageencryption.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/util/virstorageencryption.h b/src/util/virstorageencryption.h index a107161f0f..05a7bffdfc 100644 --- a/src/util/virstorageencryption.h +++ b/src/util/virstorageencryption.h @@ -79,6 +79,7 @@ virStorageEncryptionPtr virStorageEncryptionCopy(const virStorageEncryption *src ATTRIBUTE_NONNULL(1); void virStorageEncryptionFree(virStorageEncryptionPtr enc); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virStorageEncryption, virStorageEncryptionFree); virStorageEncryptionPtr virStorageEncryptionParseNode(xmlNodePtr node, xmlXPathContextPtr ctxt); -- 2.26.2
On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote:
This will open an opportunity to modernize virDomainDiskDefParseXML() in the next patch.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/util/virstorageencryption.h | 1 + 1 file changed, 1 insertion(+)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
Register an AUTOPTR_CLEANUP_FUNC for virDomainDiskDefPtr, then use g_autoptr() in virDomainDiskDef and virStorageEncryption pointers to get rid of the 'cleanup' and 'error' labels. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 95 +++++++++++++++++++----------------------- src/conf/domain_conf.h | 1 + 2 files changed, 45 insertions(+), 51 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a8d82f4733..e21f353595 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10522,11 +10522,11 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, xmlXPathContextPtr ctxt, unsigned int flags) { - virDomainDiskDefPtr def; + g_autoptr(virDomainDiskDef) def = NULL; xmlNodePtr cur; VIR_XPATH_NODE_AUTORESTORE(ctxt) bool source = false; - virStorageEncryptionPtr encryption = NULL; + g_autoptr(virStorageEncryption) encryption = NULL; g_autoptr(virStorageAuthDef) authdef = NULL; g_autofree char *tmp = NULL; g_autofree char *snapshot = NULL; @@ -10558,7 +10558,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, (def->src->type = virStorageTypeFromString(tmp)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown disk type '%s'"), tmp); - goto error; + return NULL; } VIR_FREE(tmp); @@ -10566,7 +10566,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, (def->device = virDomainDiskDeviceTypeFromString(tmp)) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown disk device '%s'"), tmp); - goto error; + return NULL; } VIR_FREE(tmp); @@ -10574,7 +10574,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, (def->model = virDomainDiskModelTypeFromString(tmp)) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown disk model '%s'"), tmp); - goto error; + return NULL; } VIR_FREE(tmp); @@ -10589,7 +10589,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, if (!source && virXMLNodeNameEqual(cur, "source")) { if (virDomainStorageSourceParse(cur, ctxt, def->src, flags, xmlopt) < 0) - goto error; + return NULL; source = true; @@ -10599,7 +10599,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, (tmp = virXMLPropString(cur, "index")) && virStrToLong_uip(tmp, NULL, 10, &def->src->id) < 0) { virReportError(VIR_ERR_XML_ERROR, _("invalid disk index '%s'"), tmp); - goto error; + return NULL; } VIR_FREE(tmp); } else if (!target && @@ -10619,7 +10619,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, domain_name = virXMLPropString(cur, "name"); } else if (virXMLNodeNameEqual(cur, "geometry")) { if (virDomainDiskDefGeometryParse(def, cur) < 0) - goto error; + return NULL; } else if (virXMLNodeNameEqual(cur, "blockio")) { logical_block_size = virXMLPropString(cur, "logical_block_size"); @@ -10629,7 +10629,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid logical block size '%s'"), logical_block_size); - goto error; + return NULL; } physical_block_size = virXMLPropString(cur, "physical_block_size"); @@ -10639,28 +10639,28 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid physical block size '%s'"), physical_block_size); - goto error; + return NULL; } } else if (!virDomainDiskGetDriver(def) && virXMLNodeNameEqual(cur, "driver")) { if (virDomainVirtioOptionsParseXML(cur, &def->virtio) < 0) - goto error; + return NULL; if (virDomainDiskDefDriverParseXML(def, cur) < 0) - goto error; + return NULL; } else if (!def->mirror && virXMLNodeNameEqual(cur, "mirror") && !(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE)) { if (virDomainDiskDefMirrorParse(def, cur, ctxt, flags, xmlopt) < 0) - goto error; + return NULL; } else if (!authdef && virXMLNodeNameEqual(cur, "auth")) { if (!(authdef = virStorageAuthDefParse(cur, ctxt))) - goto error; + return NULL; def->diskElementAuth = true; } else if (virXMLNodeNameEqual(cur, "iotune")) { if (virDomainDiskDefIotuneParse(def, ctxt) < 0) - goto error; + return NULL; } else if (virXMLNodeNameEqual(cur, "readonly")) { def->src->readonly = true; } else if (virXMLNodeNameEqual(cur, "shareable")) { @@ -10670,52 +10670,52 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, } else if (!encryption && virXMLNodeNameEqual(cur, "encryption")) { if (!(encryption = virStorageEncryptionParseNode(cur, ctxt))) - goto error; + return NULL; def->diskElementEnc = true; } else if (!serial && virXMLNodeNameEqual(cur, "serial")) { if (!(serial = virXMLNodeContentString(cur))) - goto error; + return NULL; } else if (!wwn && virXMLNodeNameEqual(cur, "wwn")) { if (!(wwn = virXMLNodeContentString(cur))) - goto error; + return NULL; if (!virValidateWWN(wwn)) - goto error; + return NULL; } else if (!vendor && virXMLNodeNameEqual(cur, "vendor")) { if (!(vendor = virXMLNodeContentString(cur))) - goto error; + return NULL; if (strlen(vendor) > VENDOR_LEN) { virReportError(VIR_ERR_XML_ERROR, "%s", _("disk vendor is more than 8 characters")); - goto error; + return NULL; } if (!virStringIsPrintable(vendor)) { virReportError(VIR_ERR_XML_ERROR, "%s", _("disk vendor is not printable string")); - goto error; + return NULL; } } else if (!product && virXMLNodeNameEqual(cur, "product")) { if (!(product = virXMLNodeContentString(cur))) - goto error; + return NULL; if (strlen(product) > PRODUCT_LEN) { virReportError(VIR_ERR_XML_ERROR, "%s", _("disk product is more than 16 characters")); - goto error; + return NULL; } if (!virStringIsPrintable(product)) { virReportError(VIR_ERR_XML_ERROR, "%s", _("disk product is not printable string")); - goto error; + return NULL; } } else if (virXMLNodeNameEqual(cur, "boot")) { /* boot is parsed as part of virDomainDeviceInfoParseXML */ @@ -10741,7 +10741,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, (flags & VIR_DOMAIN_DEF_PARSE_DISK_SOURCE))) { virReportError(VIR_ERR_NO_SOURCE, target ? "%s" : NULL, target); - goto error; + return NULL; } if (!target && !(flags & VIR_DOMAIN_DEF_PARSE_DISK_SOURCE)) { @@ -10754,7 +10754,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, } else { virReportError(VIR_ERR_NO_TARGET, def->src->path ? "%s" : NULL, def->src->path); } - goto error; + return NULL; } if (!(flags & VIR_DOMAIN_DEF_PARSE_DISK_SOURCE)) { @@ -10762,7 +10762,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, !STRPREFIX(target, "fd")) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid floppy device name: %s"), target); - goto error; + return NULL; } /* Force CDROM to be listed as read only */ @@ -10778,7 +10778,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, !STRPREFIX((const char *)target, "ubd")) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid harddisk device name: %s"), target); - goto error; + return NULL; } } @@ -10788,7 +10788,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown disk snapshot setting '%s'"), snapshot); - goto error; + return NULL; } } else if (def->src->readonly) { def->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE; @@ -10799,7 +10799,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, virReportError(VIR_ERR_XML_ERROR, _("unknown disk rawio setting '%s'"), rawio); - goto error; + return NULL; } } @@ -10807,7 +10807,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, if ((def->sgio = virDomainDeviceSGIOTypeFromString(sgio)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown disk sgio mode '%s'"), sgio); - goto error; + return NULL; } } @@ -10815,7 +10815,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, if ((def->bus = virDomainDiskBusTypeFromString(bus)) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown disk bus type '%s'"), bus); - goto error; + return NULL; } } else { if (def->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) { @@ -10840,14 +10840,14 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, if ((def->tray_status = virDomainDiskTrayTypeFromString(tray)) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown disk tray status '%s'"), tray); - goto error; + return NULL; } if (def->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY && def->device != VIR_DOMAIN_DISK_DEVICE_CDROM) { virReportError(VIR_ERR_XML_ERROR, "%s", _("tray is only valid for cdrom and floppy")); - goto error; + return NULL; } } @@ -10855,13 +10855,13 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, if ((def->removable = virTristateSwitchTypeFromString(removable)) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown disk removable status '%s'"), removable); - goto error; + return NULL; } } if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, flags | VIR_DOMAIN_DEF_PARSE_ALLOW_BOOT) < 0) { - goto error; + return NULL; } if (startupPolicy) { @@ -10871,7 +10871,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown startupPolicy value '%s'"), startupPolicy); - goto error; + return NULL; } def->startupPolicy = val; } @@ -10884,7 +10884,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("an <auth> definition already found for " "disk source")); - goto error; + return NULL; } def->src->auth = g_steal_pointer(&authdef); @@ -10897,7 +10897,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("an <encryption> definition already found for " "disk source")); - goto error; + return NULL; } def->src->encryption = g_steal_pointer(&encryption); @@ -10909,23 +10909,16 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, def->product = g_steal_pointer(&product); if (virDomainDiskBackingStoreParse(ctxt, def->src, flags, xmlopt) < 0) - goto error; + return NULL; if (flags & VIR_DOMAIN_DEF_PARSE_STATUS && virDomainDiskDefParsePrivateData(ctxt, def, xmlopt) < 0) - goto error; + return NULL; if (virDomainDiskDefParseValidate(def) < 0) - goto error; - - cleanup: - virStorageEncryptionFree(encryption); - return def; + return NULL; - error: - virDomainDiskDefFree(def); - def = NULL; - goto cleanup; + return g_steal_pointer(&def); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ff82da7f7e..cef17efe73 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3040,6 +3040,7 @@ const char *virDomainInputDefGetPath(virDomainInputDefPtr input); void virDomainInputDefFree(virDomainInputDefPtr def); virDomainDiskDefPtr virDomainDiskDefNew(virDomainXMLOptionPtr xmlopt); void virDomainDiskDefFree(virDomainDiskDefPtr def); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainDiskDef, virDomainDiskDefFree); void virDomainLeaseDefFree(virDomainLeaseDefPtr def); int virDomainDiskGetType(virDomainDiskDefPtr def); void virDomainDiskSetType(virDomainDiskDefPtr def, int type); -- 2.26.2
On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote:
Register an AUTOPTR_CLEANUP_FUNC for virDomainDiskDefPtr, then use g_autoptr() in virDomainDiskDef and virStorageEncryption pointers to get rid of the 'cleanup' and 'error' labels.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 95 +++++++++++++++++++----------------------- src/conf/domain_conf.h | 1 + 2 files changed, 45 insertions(+), 51 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
The 'tray' check isn't a XML parse specific code and can be pushed to post, in virDomainDiskDefPostParse(). 'vendor' and 'product' string sizes are already checked by the domaincommon.rng schema, but can be of use in post parse time since not all scenarios will go through the XML parsing. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 47 ++++++++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 22 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e21f353595..6623abad73 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5253,6 +5253,9 @@ virDomainDiskExpandGroupIoTune(virDomainDiskDefPtr disk, } +#define VENDOR_LEN 8 +#define PRODUCT_LEN 16 + static int virDomainDiskDefPostParse(virDomainDiskDefPtr disk, const virDomainDef *def, @@ -5277,6 +5280,28 @@ virDomainDiskDefPostParse(virDomainDiskDefPtr disk, } } + if (disk->tray_status && + disk->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY && + disk->device != VIR_DOMAIN_DISK_DEVICE_CDROM) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("tray is only valid for cdrom and floppy")); + return -1; + } + + if (disk->vendor && strlen(disk->vendor) > VENDOR_LEN) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("disk vendor is more than %d characters"), + VENDOR_LEN); + return -1; + } + + if (disk->product && strlen(disk->product) > PRODUCT_LEN) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("disk product is more than %d characters"), + PRODUCT_LEN); + return -1; + } + if (disk->src->type == VIR_STORAGE_TYPE_NETWORK && disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI) { virDomainPostParseCheckISCSIPath(&disk->src->path); @@ -10513,9 +10538,6 @@ virDomainDiskDefParsePrivateData(xmlXPathContextPtr ctxt, } -#define VENDOR_LEN 8 -#define PRODUCT_LEN 16 - static virDomainDiskDefPtr virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, xmlNodePtr node, @@ -10690,12 +10712,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, if (!(vendor = virXMLNodeContentString(cur))) return NULL; - if (strlen(vendor) > VENDOR_LEN) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("disk vendor is more than 8 characters")); - return NULL; - } - if (!virStringIsPrintable(vendor)) { virReportError(VIR_ERR_XML_ERROR, "%s", _("disk vendor is not printable string")); @@ -10706,12 +10722,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, if (!(product = virXMLNodeContentString(cur))) return NULL; - if (strlen(product) > PRODUCT_LEN) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("disk product is more than 16 characters")); - return NULL; - } - if (!virStringIsPrintable(product)) { virReportError(VIR_ERR_XML_ERROR, "%s", _("disk product is not printable string")); @@ -10842,13 +10852,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, _("unknown disk tray status '%s'"), tray); return NULL; } - - if (def->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY && - def->device != VIR_DOMAIN_DISK_DEVICE_CDROM) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("tray is only valid for cdrom and floppy")); - return NULL; - } } if (removable) { -- 2.26.2
This check is not related to XML parsing and can be moved to post parse time. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6623abad73..09b284733a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5438,6 +5438,20 @@ virDomainMemoryDefPostParse(virDomainMemoryDefPtr mem, } +static int +virDomainSmartcardDefPostParse(virDomainSmartcardDefPtr smartcard) +{ + if (smartcard->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && + smartcard->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCID) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Controllers must use the 'ccid' address type")); + return -1; + } + + return 0; +} + + static int virDomainDeviceDefPostParseCommon(virDomainDeviceDefPtr dev, const virDomainDef *def, @@ -5486,6 +5500,10 @@ virDomainDeviceDefPostParseCommon(virDomainDeviceDefPtr dev, ret = virDomainMemoryDefPostParse(dev->data.memory, def); break; + case VIR_DOMAIN_DEVICE_SMARTCARD: + ret = virDomainSmartcardDefPostParse(dev->data.smartcard); + break; + case VIR_DOMAIN_DEVICE_LEASE: case VIR_DOMAIN_DEVICE_FS: case VIR_DOMAIN_DEVICE_INPUT: @@ -5494,7 +5512,6 @@ virDomainDeviceDefPostParseCommon(virDomainDeviceDefPtr dev, case VIR_DOMAIN_DEVICE_GRAPHICS: case VIR_DOMAIN_DEVICE_HUB: case VIR_DOMAIN_DEVICE_REDIRDEV: - case VIR_DOMAIN_DEVICE_SMARTCARD: case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: case VIR_DOMAIN_DEVICE_SHMEM: @@ -13702,12 +13719,6 @@ virDomainSmartcardDefParseXML(virDomainXMLOptionPtr xmlopt, if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, flags) < 0) goto error; - if (def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && - def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCID) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Controllers must use the 'ccid' address type")); - goto error; - } return def; -- 2.26.2
Register a AUTOPTR_CLEANUP_FUNC for virDomainSmartcardDef and use g_autoptr() to eliminate the 'error' label. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 34 +++++++++++++++------------------- src/conf/domain_conf.h | 1 + 2 files changed, 16 insertions(+), 19 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 09b284733a..566185fe43 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13617,7 +13617,7 @@ virDomainSmartcardDefParseXML(virDomainXMLOptionPtr xmlopt, unsigned int flags) { xmlNodePtr cur; - virDomainSmartcardDefPtr def; + g_autoptr(virDomainSmartcardDef) def = NULL; size_t i; g_autofree char *mode = NULL; g_autofree char *type = NULL; @@ -13628,13 +13628,13 @@ virDomainSmartcardDefParseXML(virDomainXMLOptionPtr xmlopt, if (mode == NULL) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing smartcard device mode")); - goto error; + return NULL; } if ((def->type = virDomainSmartcardTypeFromString(mode)) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown smartcard device mode: %s"), mode); - goto error; + return NULL; } switch (def->type) { @@ -13651,23 +13651,23 @@ virDomainSmartcardDefParseXML(virDomainXMLOptionPtr xmlopt, virReportError(VIR_ERR_XML_ERROR, "%s", _("host-certificates mode needs " "exactly three certificates")); - goto error; + return NULL; } if (!(def->data.cert.file[i] = virXMLNodeContentString(cur))) - goto error; + return NULL; i++; } else if (cur->type == XML_ELEMENT_NODE && virXMLNodeNameEqual(cur, "database") && !def->data.cert.database) { if (!(def->data.cert.database = virXMLNodeContentString(cur))) - goto error; + return NULL; if (*def->data.cert.database != '/') { virReportError(VIR_ERR_XML_ERROR, _("expecting absolute path: %s"), def->data.cert.database); - goto error; + return NULL; } } cur = cur->next; @@ -13676,7 +13676,7 @@ virDomainSmartcardDefParseXML(virDomainXMLOptionPtr xmlopt, virReportError(VIR_ERR_XML_ERROR, "%s", _("host-certificates mode needs " "exactly three certificates")); - goto error; + return NULL; } break; @@ -13686,23 +13686,23 @@ virDomainSmartcardDefParseXML(virDomainXMLOptionPtr xmlopt, virReportError(VIR_ERR_XML_ERROR, "%s", _("passthrough mode requires a character " "device type attribute")); - goto error; + return NULL; } if (!(def->data.passthru = virDomainChrSourceDefNew(xmlopt))) - goto error; + return NULL; if ((def->data.passthru->type = virDomainChrTypeFromString(type)) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown type presented to host for " "character device: %s"), type); - goto error; + return NULL; } cur = node->children; if (virDomainChrSourceDefParseXML(def->data.passthru, cur, flags, NULL, ctxt) < 0) - goto error; + return NULL; if (def->data.passthru->type == VIR_DOMAIN_CHR_TYPE_SPICEVMC) { def->data.passthru->data.spicevmc @@ -13714,17 +13714,13 @@ virDomainSmartcardDefParseXML(virDomainXMLOptionPtr xmlopt, default: virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("unknown smartcard mode")); - goto error; + return NULL; } if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, flags) < 0) - goto error; - - return def; + return NULL; - error: - virDomainSmartcardDefFree(def); - return NULL; + return g_steal_pointer(&def); } /* Parse the XML definition for a TPM device diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index cef17efe73..694f015011 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3070,6 +3070,7 @@ void virDomainVsockDefFree(virDomainVsockDefPtr vsock); G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainVsockDef, virDomainVsockDefFree); void virDomainNetDefFree(virDomainNetDefPtr def); void virDomainSmartcardDefFree(virDomainSmartcardDefPtr def); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainSmartcardDef, virDomainSmartcardDefFree); void virDomainChrDefFree(virDomainChrDefPtr def); int virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest, virDomainChrSourceDefPtr src); -- 2.26.2
On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote:
Register a AUTOPTR_CLEANUP_FUNC for virDomainSmartcardDef and use g_autoptr() to eliminate the 'error' label.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 34 +++++++++++++++------------------- src/conf/domain_conf.h | 1 + 2 files changed, 16 insertions(+), 19 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
The 'error' label is just doing a 'return -1'. There's also a couple of 'VIR_FREE(nodes)' calls that are happening right before exiting on error, but 'nodes' is already set for autocleanup. These calls can also be removed. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 77 ++++++++++++++++++++---------------------- 1 file changed, 36 insertions(+), 41 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 566185fe43..cced9aee50 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -21406,7 +21406,7 @@ virDomainDefTunablesParse(virDomainDefPtr def, if ((n = virXPathNodeSet("./blkiotune/device", ctxt, &nodes)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot extract blkiotune nodes")); - goto error; + return -1; } if (n) def->blkio.devices = g_new0(virBlkioDevice, n); @@ -21414,7 +21414,7 @@ virDomainDefTunablesParse(virDomainDefPtr def, for (i = 0; i < n; i++) { if (virDomainBlkioDeviceParseXML(nodes[i], &def->blkio.devices[i]) < 0) - goto error; + return -1; def->blkio.ndevices++; for (j = 0; j < i; j++) { if (STREQ(def->blkio.devices[j].path, @@ -21422,7 +21422,7 @@ virDomainDefTunablesParse(virDomainDefPtr def, virReportError(VIR_ERR_XML_ERROR, _("duplicate blkio device path '%s'"), def->blkio.devices[i].path); - goto error; + return -1; } } } @@ -21431,32 +21431,32 @@ virDomainDefTunablesParse(virDomainDefPtr def, /* Extract other memory tunables */ if (virDomainParseMemoryLimit("./memtune/hard_limit[1]", NULL, ctxt, &def->mem.hard_limit) < 0) - goto error; + return -1; if (virDomainParseMemoryLimit("./memtune/soft_limit[1]", NULL, ctxt, &def->mem.soft_limit) < 0) - goto error; + return -1; if (virDomainParseMemory("./memtune/min_guarantee[1]", NULL, ctxt, &def->mem.min_guarantee, false, false) < 0) - goto error; + return -1; if (virDomainParseMemoryLimit("./memtune/swap_hard_limit[1]", NULL, ctxt, &def->mem.swap_hard_limit) < 0) - goto error; + return -1; if (virDomainVcpuParse(def, ctxt, xmlopt) < 0) - goto error; + return -1; if (virDomainDefParseIOThreads(def, ctxt) < 0) - goto error; + return -1; /* Extract cpu tunables. */ if ((n = virXPathULongLong("string(./cputune/shares[1])", ctxt, &def->cputune.shares)) < -1) { virReportError(VIR_ERR_XML_ERROR, "%s", _("can't parse cputune shares value")); - goto error; + return -1; } else if (n == 0) { def->cputune.sharesSpecified = true; } @@ -21465,42 +21465,42 @@ virDomainDefTunablesParse(virDomainDefPtr def, &def->cputune.period) < -1) { virReportError(VIR_ERR_XML_ERROR, "%s", _("can't parse cputune period value")); - goto error; + return -1; } if (virXPathLongLong("string(./cputune/quota[1])", ctxt, &def->cputune.quota) < -1) { virReportError(VIR_ERR_XML_ERROR, "%s", _("can't parse cputune quota value")); - goto error; + return -1; } if (virXPathULongLong("string(./cputune/global_period[1])", ctxt, &def->cputune.global_period) < -1) { virReportError(VIR_ERR_XML_ERROR, "%s", _("can't parse cputune global period value")); - goto error; + return -1; } if (virXPathLongLong("string(./cputune/global_quota[1])", ctxt, &def->cputune.global_quota) < -1) { virReportError(VIR_ERR_XML_ERROR, "%s", _("can't parse cputune global quota value")); - goto error; + return -1; } if (virXPathULongLong("string(./cputune/emulator_period[1])", ctxt, &def->cputune.emulator_period) < -1) { virReportError(VIR_ERR_XML_ERROR, "%s", _("can't parse cputune emulator period value")); - goto error; + return -1; } if (virXPathLongLong("string(./cputune/emulator_quota[1])", ctxt, &def->cputune.emulator_quota) < -1) { virReportError(VIR_ERR_XML_ERROR, "%s", _("can't parse cputune emulator quota value")); - goto error; + return -1; } @@ -21508,41 +21508,40 @@ virDomainDefTunablesParse(virDomainDefPtr def, &def->cputune.iothread_period) < -1) { virReportError(VIR_ERR_XML_ERROR, "%s", _("can't parse cputune iothread period value")); - goto error; + return -1; } if (virXPathLongLong("string(./cputune/iothread_quota[1])", ctxt, &def->cputune.iothread_quota) < -1) { virReportError(VIR_ERR_XML_ERROR, "%s", _("can't parse cputune iothread quota value")); - goto error; + return -1; } if ((n = virXPathNodeSet("./cputune/vcpupin", ctxt, &nodes)) < 0) - goto error; + return -1; for (i = 0; i < n; i++) { if (virDomainVcpuPinDefParseXML(def, nodes[i])) - goto error; + return -1; } VIR_FREE(nodes); if ((n = virXPathNodeSet("./cputune/emulatorpin", ctxt, &nodes)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot extract emulatorpin nodes")); - goto error; + return -1; } if (n) { if (n > 1) { virReportError(VIR_ERR_XML_ERROR, "%s", _("only one emulatorpin is supported")); - VIR_FREE(nodes); - goto error; + return -1; } if (!(def->cputune.emulatorpin = virDomainEmulatorPinDefParseXML(nodes[0]))) - goto error; + return -1; } VIR_FREE(nodes); @@ -21550,86 +21549,82 @@ virDomainDefTunablesParse(virDomainDefPtr def, if ((n = virXPathNodeSet("./cputune/iothreadpin", ctxt, &nodes)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot extract iothreadpin nodes")); - goto error; + return -1; } for (i = 0; i < n; i++) { if (virDomainIOThreadPinDefParseXML(nodes[i], def) < 0) - goto error; + return -1; } VIR_FREE(nodes); if ((n = virXPathNodeSet("./cputune/vcpusched", ctxt, &nodes)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot extract vcpusched nodes")); - goto error; + return -1; } for (i = 0; i < n; i++) { if (virDomainVcpuThreadSchedParse(nodes[i], def) < 0) - goto error; + return -1; } VIR_FREE(nodes); if ((n = virXPathNodeSet("./cputune/iothreadsched", ctxt, &nodes)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot extract iothreadsched nodes")); - goto error; + return -1; } for (i = 0; i < n; i++) { if (virDomainIOThreadSchedParse(nodes[i], def) < 0) - goto error; + return -1; } VIR_FREE(nodes); if ((n = virXPathNodeSet("./cputune/emulatorsched", ctxt, &nodes)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot extract emulatorsched nodes")); - goto error; + return -1; } if (n) { if (n > 1) { virReportError(VIR_ERR_XML_ERROR, "%s", _("only one emulatorsched is supported")); - VIR_FREE(nodes); - goto error; + return -1; } if (virDomainEmulatorSchedParse(nodes[0], def) < 0) - goto error; + return -1; } VIR_FREE(nodes); if ((n = virXPathNodeSet("./cputune/cachetune", ctxt, &nodes)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot extract cachetune nodes")); - goto error; + return -1; } for (i = 0; i < n; i++) { if (virDomainCachetuneDefParse(def, ctxt, nodes[i], flags) < 0) - goto error; + return -1; } VIR_FREE(nodes); if ((n = virXPathNodeSet("./cputune/memorytune", ctxt, &nodes)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot extract memorytune nodes")); - goto error; + return -1; } for (i = 0; i < n; i++) { if (virDomainMemorytuneDefParse(def, ctxt, nodes[i], flags) < 0) - goto error; + return -1; } VIR_FREE(nodes); return 0; - - error: - return -1; } -- 2.26.2
On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote:
The 'error' label is just doing a 'return -1'.
There's also a couple of 'VIR_FREE(nodes)' calls that are happening right before exiting on error, but 'nodes' is already set for autocleanup. These calls can also be removed.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 77 ++++++++++++++++++++---------------------- 1 file changed, 36 insertions(+), 41 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
Use this check to create a virDomainDefTunablesPostParse() function, that is called on post parse time. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 35 +++++++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 10 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index cced9aee50..220224cebe 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5933,6 +5933,27 @@ virDomainDefBootPostParse(virDomainDefPtr def) } +static int +virDomainDefTunablesPostParse(virDomainDefPtr def) +{ + size_t i, j; + + for (i = 0; i < def->blkio.ndevices; i++) { + for (j = 0; j < i; j++) { + if (STREQ(def->blkio.devices[j].path, + def->blkio.devices[i].path)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("duplicate blkio device path '%s'"), + def->blkio.devices[i].path); + return -1; + } + } + } + + return 0; +} + + static int virDomainDefPostParseVideo(virDomainDefPtr def, void *opaque) @@ -6020,6 +6041,9 @@ virDomainDefPostParseCommon(virDomainDefPtr def, if (virDomainDefBootPostParse(def) < 0) return -1; + if (virDomainDefTunablesPostParse(def) < 0) + return -1; + if (def->os.type == VIR_DOMAIN_OSTYPE_HVM && !(data->xmlopt->config.features & VIR_DOMAIN_DEF_FEATURE_NO_BOOT_ORDER) && virDomainDefBootOrderPostParse(def) < 0) @@ -21395,7 +21419,7 @@ virDomainDefTunablesParse(virDomainDefPtr def, unsigned int flags) { g_autofree xmlNodePtr *nodes = NULL; - size_t i, j; + size_t i; int n; /* Extract blkio cgroup tunables */ @@ -21416,15 +21440,6 @@ virDomainDefTunablesParse(virDomainDefPtr def, &def->blkio.devices[i]) < 0) return -1; def->blkio.ndevices++; - for (j = 0; j < i; j++) { - if (STREQ(def->blkio.devices[j].path, - def->blkio.devices[i].path)) { - virReportError(VIR_ERR_XML_ERROR, - _("duplicate blkio device path '%s'"), - def->blkio.devices[i].path); - return -1; - } - } } VIR_FREE(nodes); -- 2.26.2
virDomainControllerDefParseXML() does a lot of checks with virDomainPCIControllerOpts parameters that can be moved to post parse time, sharing the logic with other use cases that does not rely on XML parsing. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 89 +++++++++++-------- .../pseries-default-phb-numa-node.err | 2 +- 2 files changed, 52 insertions(+), 39 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 220224cebe..9ca979b345 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5379,6 +5379,57 @@ virDomainControllerDefPostParse(virDomainControllerDefPtr cdev) return -1; } + if (cdev->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { + virDomainPCIControllerOpts pciOpts = cdev->opts.pciopts; + + if (pciOpts.chassisNr != -1) { + if (pciOpts.chassisNr < 1 || pciOpts.chassisNr > 255) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("PCI controller chassisNr '%d' out of range " + "- must be 1-255"), + pciOpts.chassisNr); + return -1; + } + } + + if (pciOpts.chassis != -1) { + if (pciOpts.chassis < 0 || pciOpts.chassis > 255) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("PCI controller chassis '%d' out of range " + "- must be 0-255"), + pciOpts.chassis); + return -1; + } + } + + if (pciOpts.port != -1) { + if (pciOpts.port < 0 || pciOpts.port > 255) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("PCI controller port '%d' out of range " + "- must be 0-255"), + pciOpts.port); + return -1; + } + } + + if (pciOpts.busNr != -1) { + if (pciOpts.busNr < 1 || pciOpts.busNr > 254) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("PCI controller busNr '%d' out of range " + "- must be 1-254"), + pciOpts.busNr); + return -1; + } + } + + if (pciOpts.numaNode >= 0 && cdev->idx == 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("The PCI controller with index=0 can't " + "be associated with a NUMA node")); + return -1; + } + } + return 0; } @@ -11416,14 +11467,6 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr xmlopt, chassisNr); goto error; } - if (def->opts.pciopts.chassisNr < 1 || - def->opts.pciopts.chassisNr > 255) { - virReportError(VIR_ERR_XML_ERROR, - _("PCI controller chassisNr '%s' out of range " - "- must be 1-255"), - chassisNr); - goto error; - } } if (chassis) { if (virStrToLong_i(chassis, NULL, 0, @@ -11433,14 +11476,6 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr xmlopt, chassis); goto error; } - if (def->opts.pciopts.chassis < 0 || - def->opts.pciopts.chassis > 255) { - virReportError(VIR_ERR_XML_ERROR, - _("PCI controller chassis '%s' out of range " - "- must be 0-255"), - chassis); - goto error; - } } if (port) { if (virStrToLong_i(port, NULL, 0, @@ -11450,14 +11485,6 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr xmlopt, port); goto error; } - if (def->opts.pciopts.port < 0 || - def->opts.pciopts.port > 255) { - virReportError(VIR_ERR_XML_ERROR, - _("PCI controller port '%s' out of range " - "- must be 0-255"), - port); - goto error; - } } if (busNr) { if (virStrToLong_i(busNr, NULL, 0, @@ -11467,14 +11494,6 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr xmlopt, busNr); goto error; } - if (def->opts.pciopts.busNr < 1 || - def->opts.pciopts.busNr > 254) { - virReportError(VIR_ERR_XML_ERROR, - _("PCI controller busNr '%s' out of range " - "- must be 1-254"), - busNr); - goto error; - } } if (targetIndex) { if (virStrToLong_i(targetIndex, NULL, 0, @@ -11487,12 +11506,6 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr xmlopt, } } if (numaNode >= 0) { - if (def->idx == 0) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("The PCI controller with index=0 can't " - "be associated with a NUMA node")); - goto error; - } def->opts.pciopts.numaNode = numaNode; } if (hotplug) { diff --git a/tests/qemuxml2argvdata/pseries-default-phb-numa-node.err b/tests/qemuxml2argvdata/pseries-default-phb-numa-node.err index 5d11109317..20dade0530 100644 --- a/tests/qemuxml2argvdata/pseries-default-phb-numa-node.err +++ b/tests/qemuxml2argvdata/pseries-default-phb-numa-node.err @@ -1 +1 @@ -XML error: The PCI controller with index=0 can't be associated with a NUMA node +unsupported configuration: The PCI controller with index=0 can't be associated with a NUMA node -- 2.26.2
Move the check made in virDomainControllerDefParseXML() to virDomainControllerDefPostParse(). Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 16 ++++++++++------ tests/qemuxml2argvdata/pci-root-address.err | 2 +- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9ca979b345..c17f5479ba 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5382,6 +5382,16 @@ virDomainControllerDefPostParse(virDomainControllerDefPtr cdev) if (cdev->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { virDomainPCIControllerOpts pciOpts = cdev->opts.pciopts; + if (cdev->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT || + cdev->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) { + if (cdev->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("pci-root and pcie-root controllers " + "should not have an address")); + return -1; + } + } + if (pciOpts.chassisNr != -1) { if (pciOpts.chassisNr < 1 || pciOpts.chassisNr > 255) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -11423,12 +11433,6 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr xmlopt, case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: { unsigned long long bytes; - if (def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("pci-root and pcie-root controllers should not " - "have an address")); - goto error; - } if ((rc = virParseScaledValue("./pcihole64", NULL, ctxt, &bytes, 1024, 1024ULL * ULONG_MAX, false)) < 0) diff --git a/tests/qemuxml2argvdata/pci-root-address.err b/tests/qemuxml2argvdata/pci-root-address.err index 53dad81985..ffe5438224 100644 --- a/tests/qemuxml2argvdata/pci-root-address.err +++ b/tests/qemuxml2argvdata/pci-root-address.err @@ -1 +1 @@ -XML error: pci-root and pcie-root controllers should not have an address +unsupported configuration: pci-root and pcie-root controllers should not have an address -- 2.26.2
Let's register AUTOPTR_CLEANUP_FUNC for virDomainControllerDefPtr and modernize this function, removing the 'error' label using g_autoptr(). Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 60 ++++++++++++++++++++---------------------- src/conf/domain_conf.h | 1 + 2 files changed, 29 insertions(+), 32 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c17f5479ba..5630f72096 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11226,7 +11226,7 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr xmlopt, xmlXPathContextPtr ctxt, unsigned int flags) { - virDomainControllerDefPtr def = NULL; + g_autoptr(virDomainControllerDef) def = NULL; int type = 0; xmlNodePtr cur = NULL; bool processedModel = false; @@ -11259,19 +11259,19 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr xmlopt, if ((type = virDomainControllerTypeFromString(typeStr)) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unknown controller type '%s'"), typeStr); - goto error; + return NULL; } } if (!(def = virDomainControllerDefNew(type))) - goto error; + return NULL; model = virXMLPropString(node, "model"); if (model) { if ((def->model = virDomainControllerModelTypeFromString(def, model)) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unknown model type '%s'"), model); - goto error; + return NULL; } } @@ -11282,7 +11282,7 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr xmlopt, idxVal > INT_MAX) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot parse controller index %s"), idx); - goto error; + return NULL; } def->idx = idxVal; } @@ -11298,13 +11298,13 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr xmlopt, iothread = virXMLPropString(cur, "iothread"); if (virDomainVirtioOptionsParseXML(cur, &def->virtio) < 0) - goto error; + return NULL; } else if (virXMLNodeNameEqual(cur, "model")) { if (processedModel) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Multiple <model> elements in " "controller definition not allowed")); - goto error; + return NULL; } modelName = virXMLPropString(cur, "name"); processedModel = true; @@ -11313,7 +11313,7 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr xmlopt, virReportError(VIR_ERR_XML_ERROR, "%s", _("Multiple <target> elements in " "controller definition not allowed")); - goto error; + return NULL; } chassisNr = virXMLPropString(cur, "chassisNr"); chassis = virXMLPropString(cur, "chassis"); @@ -11334,39 +11334,39 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr xmlopt, if (rc == -2 || (rc == 0 && numaNode < 0)) { virReportError(VIR_ERR_XML_ERROR, "%s", _("invalid NUMA node in target")); - goto error; + return NULL; } if (queues && virStrToLong_ui(queues, NULL, 10, &def->queues) < 0) { virReportError(VIR_ERR_XML_ERROR, _("Malformed 'queues' value '%s'"), queues); - goto error; + return NULL; } if (cmd_per_lun && virStrToLong_ui(cmd_per_lun, NULL, 10, &def->cmd_per_lun) < 0) { virReportError(VIR_ERR_XML_ERROR, _("Malformed 'cmd_per_lun' value '%s'"), cmd_per_lun); - goto error; + return NULL; } if (max_sectors && virStrToLong_ui(max_sectors, NULL, 10, &def->max_sectors) < 0) { virReportError(VIR_ERR_XML_ERROR, _("Malformed 'max_sectors' value %s"), max_sectors); - goto error; + return NULL; } if (ioeventfd && (def->ioeventfd = virTristateSwitchTypeFromString(ioeventfd)) < 0) { virReportError(VIR_ERR_XML_ERROR, _("Malformed 'ioeventfd' value %s"), ioeventfd); - goto error; + return NULL; } if (iothread) { if (virStrToLong_uip(iothread, NULL, 10, &def->iothread) < 0) { virReportError(VIR_ERR_XML_ERROR, _("Invalid 'iothread' value '%s'"), iothread); - goto error; + return NULL; } } @@ -11375,7 +11375,7 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr xmlopt, VIR_DEBUG("Ignoring device address for none model usb controller"); } else if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, flags) < 0) { - goto error; + return NULL; } portsStr = virXMLPropString(node, "ports"); @@ -11384,7 +11384,7 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr xmlopt, if (r != 0 || ports < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid ports: %s"), portsStr); - goto error; + return NULL; } } @@ -11399,7 +11399,7 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr xmlopt, if (r != 0 || def->opts.vioserial.vectors < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid vectors: %s"), vectors); - goto error; + return NULL; } } break; @@ -11436,7 +11436,7 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr xmlopt, if ((rc = virParseScaledValue("./pcihole64", NULL, ctxt, &bytes, 1024, 1024ULL * ULONG_MAX, false)) < 0) - goto error; + return NULL; if (rc == 1) def->opts.pciopts.pcihole64 = true; @@ -11461,7 +11461,7 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr xmlopt, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unknown PCI controller model name '%s'"), modelName); - goto error; + return NULL; } if (chassisNr) { if (virStrToLong_i(chassisNr, NULL, 0, @@ -11469,7 +11469,7 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr xmlopt, virReportError(VIR_ERR_XML_ERROR, _("Invalid chassisNr '%s' in PCI controller"), chassisNr); - goto error; + return NULL; } } if (chassis) { @@ -11478,7 +11478,7 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr xmlopt, virReportError(VIR_ERR_XML_ERROR, _("Invalid chassis '%s' in PCI controller"), chassis); - goto error; + return NULL; } } if (port) { @@ -11487,7 +11487,7 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr xmlopt, virReportError(VIR_ERR_XML_ERROR, _("Invalid port '%s' in PCI controller"), port); - goto error; + return NULL; } } if (busNr) { @@ -11496,7 +11496,7 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr xmlopt, virReportError(VIR_ERR_XML_ERROR, _("Invalid busNr '%s' in PCI controller"), busNr); - goto error; + return NULL; } } if (targetIndex) { @@ -11506,7 +11506,7 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr xmlopt, virReportError(VIR_ERR_XML_ERROR, _("Invalid target index '%s' in PCI controller"), targetIndex); - goto error; + return NULL; } } if (numaNode >= 0) { @@ -11519,7 +11519,7 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr xmlopt, virReportError(VIR_ERR_XML_ERROR, _("PCI controller unrecognized hotplug setting '%s'"), hotplug); - goto error; + return NULL; } def->opts.pciopts.hotplug = val; } @@ -11534,7 +11534,7 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr xmlopt, if (r != 0 || def->opts.xenbusopts.maxGrantFrames < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid maxGrantFrames: %s"), gntframes); - goto error; + return NULL; } } if (eventchannels) { @@ -11543,7 +11543,7 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr xmlopt, if (r != 0 || def->opts.xenbusopts.maxEventChannels < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid maxEventChannels: %s"), eventchannels); - goto error; + return NULL; } } break; @@ -11553,11 +11553,7 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr xmlopt, break; } - return def; - - error: - virDomainControllerDefFree(def); - return NULL; + return g_steal_pointer(&def); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 694f015011..34cde22965 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3060,6 +3060,7 @@ virDomainDiskDefPtr virDomainDiskFindByBusAndDst(virDomainDefPtr def, virDomainControllerDefPtr virDomainControllerDefNew(virDomainControllerType type); void virDomainControllerDefFree(virDomainControllerDefPtr def); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainControllerDef, virDomainControllerDefFree); bool virDomainControllerIsPSeriesPHB(const virDomainControllerDef *cont); virDomainFSDefPtr virDomainFSDefNew(virDomainXMLOptionPtr xmlopt); -- 2.26.2
On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote:
Let's register AUTOPTR_CLEANUP_FUNC for virDomainControllerDefPtr and modernize this function, removing the 'error' label using g_autoptr().
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 60 ++++++++++++++++++++---------------------- src/conf/domain_conf.h | 1 + 2 files changed, 29 insertions(+), 32 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
The 'error' label is just returning -1, so let's 'return -1' directly. Use g_autoptr() with virDomainControllerDefPtr to remove the need to call virDomainControllerDefFree() in the error path. There is no need to VIR_FREE(nodes) explictly since 'nodes' is using g_autofree. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5630f72096..b794611b1e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -21819,38 +21819,36 @@ virDomainDefControllersParse(virDomainDefPtr def, int n; if ((n = virXPathNodeSet("./devices/controller", ctxt, &nodes)) < 0) - goto error; + return -1; if (n) def->controllers = g_new0(virDomainControllerDefPtr, n); for (i = 0; i < n; i++) { - virDomainControllerDefPtr controller = virDomainControllerDefParseXML(xmlopt, - nodes[i], - ctxt, - flags); + g_autoptr(virDomainControllerDef) controller = NULL; + + controller = virDomainControllerDefParseXML(xmlopt, nodes[i], + ctxt, flags); if (!controller) - goto error; + return -1; /* sanitize handling of "none" usb controller */ if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) { if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE) { if (usb_other || *usb_none) { - virDomainControllerDefFree(controller); virReportError(VIR_ERR_XML_DETAIL, "%s", _("Can't add another USB controller: " "USB is disabled for this domain")); - goto error; + return -1; } *usb_none = true; } else { if (*usb_none) { - virDomainControllerDefFree(controller); virReportError(VIR_ERR_XML_DETAIL, "%s", _("Can't add another USB controller: " "USB is disabled for this domain")); - goto error; + return -1; } usb_other = true; } @@ -21859,20 +21857,16 @@ virDomainDefControllersParse(virDomainDefPtr def, usb_master = true; } - virDomainControllerInsertPreAlloced(def, controller); + virDomainControllerInsertPreAlloced(def, g_steal_pointer(&controller)); } - VIR_FREE(nodes); if (usb_other && !usb_master) { virReportError(VIR_ERR_XML_DETAIL, "%s", _("No master USB controller specified")); - goto error; + return -1; } return 0; - - error: - return -1; } static virDomainDefPtr -- 2.26.2
On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote:
The 'error' label is just returning -1, so let's 'return -1' directly.
Use g_autoptr() with virDomainControllerDefPtr to remove the need to call virDomainControllerDefFree() in the error path.
There is no need to VIR_FREE(nodes) explictly since 'nodes' is using g_autofree.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
Some occurrences in post parse functions of domain_conf.c are using VIR_ERR_XML_ERROR. Use VIR_ERR_CONFIG_UNSUPPORTED instead since these errors might not be related to a XML use case. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b794611b1e..c99c1b60d4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5266,15 +5266,15 @@ virDomainDiskDefPostParse(virDomainDiskDefPtr disk, if (virStorageSourceGetActualType(disk->src) != VIR_STORAGE_TYPE_NETWORK && disk->src->protocol != VIR_STORAGE_NET_PROTOCOL_RBD) { if (disk->src->snapshot) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("<snapshot> element is currently supported " + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("'snapshot' is currently supported " "only with 'rbd' disks")); return -1; } if (disk->src->configFile) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("<config> element is currently supported " + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("'config' is currently supported " "only with 'rbd' disks")); return -1; } @@ -5373,7 +5373,7 @@ virDomainControllerDefPostParse(virDomainControllerDefPtr cdev) cdev->model != VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI && cdev->model != VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL && cdev->model != VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL) { - virReportError(VIR_ERR_XML_ERROR, "%s", + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("'iothread' attribute only supported for " "virtio scsi controllers")); return -1; -- 2.26.2
Create a new function called virDomainDefIdMapPostParse() and use it to move these checks out of virDomainIdmapDefParseXML(). Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 42 ++++++++++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c99c1b60d4..fa21d41c4a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6015,6 +6015,29 @@ virDomainDefTunablesPostParse(virDomainDefPtr def) } +static int +virDomainDefIdMapPostParse(virDomainDefPtr def) +{ + if ((def->idmap.uidmap && !def->idmap.gidmap) || + (!def->idmap.uidmap && def->idmap.gidmap)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("uid and gid should be mapped both")); + return -1; + } + + if ((def->idmap.uidmap && def->idmap.uidmap[0].start != 0) || + (def->idmap.gidmap && def->idmap.gidmap[0].start != 0)) { + /* Root user of container hasn't been mapped to any user of host, + * return error. */ + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("You must map the root user of container")); + return -1; + } + + return 0; +} + + static int virDomainDefPostParseVideo(virDomainDefPtr def, void *opaque) @@ -6105,6 +6128,9 @@ virDomainDefPostParseCommon(virDomainDefPtr def, if (virDomainDefTunablesPostParse(def) < 0) return -1; + if (virDomainDefIdMapPostParse(def) < 0) + return -1; + if (def->os.type == VIR_DOMAIN_OSTYPE_HVM && !(data->xmlopt->config.features & VIR_DOMAIN_DEF_FEATURE_NO_BOOT_ORDER) && virDomainDefBootOrderPostParse(def) < 0) @@ -19098,15 +19124,6 @@ virDomainIdmapDefParseXML(xmlXPathContextPtr ctxt, qsort(idmap, num, sizeof(idmap[0]), virDomainIdMapEntrySort); - if (idmap[0].start != 0) { - /* Root user of container hasn't been mapped to any user of host, - * return error. */ - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("You must map the root user of container")); - VIR_FREE(idmap); - return NULL; - } - return idmap; } @@ -22595,13 +22612,6 @@ virDomainDefParseXML(xmlDocPtr xml, } VIR_FREE(nodes); - if ((def->idmap.uidmap && !def->idmap.gidmap) || - (!def->idmap.uidmap && def->idmap.gidmap)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("uid and gid should be mapped both")); - goto error; - } - if ((n = virXPathNodeSet("./sysinfo", ctxt, &nodes)) < 0) goto error; -- 2.26.2
Ping On 11/24/20 4:20 PM, Daniel Henrique Barboza wrote:
Hi,
This started as a simple NVDIMM change, then I realized there is a Gitlab work item for it [1], so I took the extra mile and did a bit more. I'll copy/paste here the motivation for this kind of change, provided by Cole in [1]:
----- The code that handles domain/VM XML parsing (virDomainDefParseXML in src/domain/domain_conf.c) has various validation checks sprinkled within it. Many of these checks should be moved out of the parse step and into virDomainDefPostParse, so they can be shared by various places in the code that build a domain definition without XML, such as converting from native vmware/virtualbox/xen formats. -----
There are still checks to be moved after this work. If this is accepted I'll update [1] with more details/tips about how to proceed with the remaining checks.
Some g_auto* cleanups were done along the way.
[1] https://gitlab.com/libvirt/libvirt/-/issues/7
Daniel Henrique Barboza (21): domain_conf.c: move NVDIMM 'labelsize' check to post parse domain_conf.c: use g_autofree in 'dev' in virDomainDefParseBootXML() domain_conf.c: modernize virDomainDefBootOrderPostParse() domain_conf.c: move boot related timeouts check to post parse domain_conf.c: do not leak 'video' in virDomainDefParseXML() domain_conf.c: move primary video check to virDomainDefPostParseVideo() domain_conf.c: use g_autoptr() with virDomainVideoDefPtr domain_conf.c: move QXL attributes check to virDomainVideoDefPostParse() virstorageencryption.h: add AUTOPTR_CLEANUP_FUNC for virStorageEncryptionPtr domain_conf: modernize virDomainDiskDefParseXML() domain_conf.c: move vendor, product and tray checks to post parse domain_conf.c: move smartcard address check to post parse domain_conf.c: modernize virDomainSmartcardDefParseXML domain_conf.c: remove 'error' label in virDomainDefTunablesParse() domain_conf.c: move duplicate blkio path check to post parse domain_conf.c: move virDomainPCIControllerOpts checks to post parse domain_conf.c: move pci-root/pcie-root address check to post parse domain_conf.c: modernize virDomainControllerDefParseXML() domain_conf.c: modernize virDomainDefControllersParse() domain_conf.c: use VIR_ERR_CONFIG_UNSUPPORTED in post parse domain_conf.c: move idmapEntry checks to post parse
src/conf/domain_conf.c | 762 ++++++++++-------- src/conf/domain_conf.h | 4 + src/util/virstorageencryption.h | 1 + tests/qemuxml2argvdata/pci-root-address.err | 2 +- .../pseries-default-phb-numa-node.err | 2 +- .../video-multiple-primaries.err | 1 + .../video-multiple-primaries.xml | 32 + tests/qemuxml2argvtest.c | 5 + 8 files changed, 451 insertions(+), 358 deletions(-) create mode 100644 tests/qemuxml2argvdata/video-multiple-primaries.err create mode 100644 tests/qemuxml2argvdata/video-multiple-primaries.xml
participants (2)
-
Daniel Henrique Barboza -
Michal Privoznik