1:20 p.m.
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
1:20 p.m.
New subject: [PATCH 01/21] domain_conf.c: move NVDIMM 'labelsize' check to post parse
Move 'labelsize' validation to virDomainMemoryDefPostParse().
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)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
12:46 p.m.
New subject: [PATCH 01/21] domain_conf.c: move NVDIMM 'labelsize' check to post parse
On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote:
Move 'labelsize' validation to
virDomainMemoryDefPostParse().
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)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
1:58 p.m.
New subject: [PATCH 01/21] domain_conf.c: move NVDIMM 'labelsize' check to post parse
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(a)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
2:20 p.m.
New subject: [PATCH 01/21] domain_conf.c: move NVDIMM 'labelsize' check to post parse
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(a)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
3:03 p.m.
New subject: [PATCH 01/21] domain_conf.c: move NVDIMM 'labelsize' check to post parse
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(a)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
3:16 p.m.
New subject: [PATCH 01/21] domain_conf.c: move NVDIMM 'labelsize' check to post parse
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(a)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
4:40 p.m.
New subject: [PATCH 01/21] domain_conf.c: move NVDIMM 'labelsize' check to post parse
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(a)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
1:20 p.m.
New subject: [PATCH 02/21] domain_conf.c: use g_autofree in 'dev' in virDomainDefParseBootXML()
This spares us of 2 explicit VIR_FREE() calls.
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)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
12:46 p.m.
New subject: [PATCH 02/21] domain_conf.c: use g_autofree in 'dev' in virDomainDefParseBootXML()
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(a)gmail.com>
---
src/conf/domain_conf.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com>
Michal
1:20 p.m.
New subject: [PATCH 03/21] domain_conf.c: modernize virDomainDefBootOrderPostParse()
Use g_autoptr() with the hash and remove the 'cleanup' label.
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)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
12:46 p.m.
New subject: [PATCH 03/21] domain_conf.c: modernize virDomainDefBootOrderPostParse()
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(a)gmail.com>
---
src/conf/domain_conf.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com>
Michal
1:20 p.m.
New subject: [PATCH 04/21] domain_conf.c: move boot related timeouts check to post parse
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(a)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
1:20 p.m.
New subject: [PATCH 05/21] domain_conf.c: do not leak 'video' in virDomainDefParseXML()
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(a)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
12:46 p.m.
New subject: [PATCH 05/21] domain_conf.c: do not leak 'video' in virDomainDefParseXML()
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(a)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(a)redhat.com>
Michal
1:20 p.m.
New subject: [PATCH 06/21] domain_conf.c: move primary video check to virDomainDefPostParseVideo()
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(a)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
1:20 p.m.
New subject: [PATCH 07/21] domain_conf.c: use g_autoptr() with virDomainVideoDefPtr
This will modernize virDomainVideoDefParseXML() and
virDomainDefAddImplicitVideo() by removing unneeded
cleanup labels.
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)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
12:46 p.m.
New subject: [PATCH 07/21] domain_conf.c: use g_autoptr() with virDomainVideoDefPtr
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(a)gmail.com>
---
src/conf/domain_conf.c | 48 ++++++++++++++++++------------------------
1 file changed, 20 insertions(+), 28 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com>
Michal
1:20 p.m.
New subject: [PATCH 08/21] domain_conf.c: move QXL attributes check to virDomainVideoDefPostParse()
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(a)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
1:20 p.m.
New subject: [PATCH 09/21] virstorageencryption.h: add AUTOPTR_CLEANUP_FUNC for virStorageEncryptionPtr
This will open an opportunity to modernize virDomainDiskDefParseXML()
in the next patch.
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)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
12:46 p.m.
New subject: [PATCH 09/21] virstorageencryption.h: add AUTOPTR_CLEANUP_FUNC for virStorageEncryptionPtr
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(a)gmail.com>
---
src/util/virstorageencryption.h | 1 +
1 file changed, 1 insertion(+)
Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com>
Michal
1:20 p.m.
New subject: [PATCH 10/21] domain_conf: modernize virDomainDiskDefParseXML()
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(a)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
12:47 p.m.
New subject: [PATCH 10/21] domain_conf: modernize virDomainDiskDefParseXML()
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(a)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(a)redhat.com>
Michal
1:20 p.m.
New subject: [PATCH 11/21] domain_conf.c: move vendor, product and tray checks to post parse
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(a)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
1:20 p.m.
New subject: [PATCH 12/21] domain_conf.c: move smartcard address check to post parse
This check is not related to XML parsing and can be moved to
post parse time.
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)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
1:20 p.m.
New subject: [PATCH 13/21] domain_conf.c: modernize virDomainSmartcardDefParseXML
Register a AUTOPTR_CLEANUP_FUNC for virDomainSmartcardDef and use
g_autoptr() to eliminate the 'error' label.
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)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
12:46 p.m.
New subject: [PATCH 13/21] domain_conf.c: modernize virDomainSmartcardDefParseXML
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(a)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(a)redhat.com>
Michal
1:20 p.m.
New subject: [PATCH 14/21] domain_conf.c: remove 'error' label in virDomainDefTunablesParse()
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(a)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
12:46 p.m.
New subject: [PATCH 14/21] domain_conf.c: remove 'error' label in virDomainDefTunablesParse()
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(a)gmail.com>
---
src/conf/domain_conf.c | 77 ++++++++++++++++++++----------------------
1 file changed, 36 insertions(+), 41 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com>
Michal
1:20 p.m.
New subject: [PATCH 15/21] domain_conf.c: move duplicate blkio path check to post parse
Use this check to create a virDomainDefTunablesPostParse()
function, that is called on post parse time.
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)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
1:20 p.m.
New subject: [PATCH 16/21] domain_conf.c: move virDomainPCIControllerOpts checks to post parse
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(a)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
1:20 p.m.
New subject: [PATCH 17/21] domain_conf.c: move pci-root/pcie-root address check to post parse
Move the check made in virDomainControllerDefParseXML() to
virDomainControllerDefPostParse().
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)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
1:20 p.m.
New subject: [PATCH 18/21] domain_conf.c: modernize virDomainControllerDefParseXML()
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(a)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
12:46 p.m.
New subject: [PATCH 18/21] domain_conf.c: modernize virDomainControllerDefParseXML()
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(a)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(a)redhat.com>
Michal
1:20 p.m.
New subject: [PATCH 19/21] domain_conf.c: modernize virDomainDefControllersParse()
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(a)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
12:46 p.m.
New subject: [PATCH 19/21] domain_conf.c: modernize virDomainDefControllersParse()
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(a)gmail.com>
---
src/conf/domain_conf.c | 26 ++++++++++----------------
1 file changed, 10 insertions(+), 16 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com>
Michal
1:20 p.m.
New subject: [PATCH 20/21] domain_conf.c: use VIR_ERR_CONFIG_UNSUPPORTED in post parse
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(a)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
1:20 p.m.
New subject: [PATCH 21/21] domain_conf.c: move idmapEntry checks to post parse
Create a new function called virDomainDefIdMapPostParse() and
use it to move these checks out of virDomainIdmapDefParseXML().
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)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
10:49 a.m.
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
1452
days inactive
1459
days old
38 comments
2 participants
participants (2)
-
Daniel Henrique Barboza -
Michal Privoznik