[PATCH v2 0/9] move checks from parse to validate callbacks
Hi, This is the respin of [1] after the reviews from Michal. Although we're pushing code to validate callbacks instead of post parse functions, the idea and motivation is still in line with [2]. [1] https://www.redhat.com/archives/libvir-list/2020-November/msg01409.html [2] https://gitlab.com/libvirt/libvirt/-/issues/7 Daniel Henrique Barboza (9): domain_conf.c: move boot related timeouts check to validate callback domain_conf.c: move primary video check to validate callback domain_conf.c: move QXL attributes check to virDomainVideoDefValidate() domain_conf.c: move vendor, product and tray checks to validate callback domain_conf.c: move smartcard address check to validate callback domain_conf.c: move blkio path check to validate callback domain_conf.c: move virDomainPCIControllerOpts checks to validate callback domain_conf.c: move pci-root/pcie-root address check to validateCB domain_conf.c: move idmapEntry checks to validate callback src/conf/domain_conf.c | 353 +++++++++++------- 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 | 14 +- 6 files changed, 268 insertions(+), 136 deletions(-) create mode 100644 tests/qemuxml2argvdata/video-multiple-primaries.err create mode 100644 tests/qemuxml2argvdata/video-multiple-primaries.xml -- 2.26.2
This patch creates a new function, virDomainDefBootValidate(), to host the validation of boot menu timeout and rebootTimeout outside of parse time. The checks in virDomainDefParseBootXML() were changed to throw VIR_ERR_XML_ERROR in case of parse error of those values. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 41 ++++++++++++++++++++++++++++++---------- tests/qemuxml2argvtest.c | 3 ++- 2 files changed, 33 insertions(+), 11 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 66ee658e7b..e53a7372fc 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7299,6 +7299,28 @@ virDomainDefOSValidate(const virDomainDef *def, } +static int +virDomainDefBootValidate(const virDomainDef *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 virDomainDefValidateInternal(const virDomainDef *def, virDomainXMLOptionPtr xmlopt) @@ -7344,6 +7366,9 @@ virDomainDefValidateInternal(const virDomainDef *def, if (virDomainDefCputuneValidate(def) < 0) return -1; + if (virDomainDefBootValidate(def) < 0) + return -1; + if (virDomainNumaDefValidate(def->numa) < 0) return -1; @@ -18867,11 +18892,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; @@ -18892,11 +18915,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; diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 7c9dc0e641..202fc83ccf 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1006,7 +1006,8 @@ mymain(void) DO_TEST("boot-menu-enable-with-timeout", QEMU_CAPS_SPLASH_TIMEOUT); DO_TEST_PARSE_ERROR("boot-menu-enable-with-timeout", NONE); - DO_TEST_PARSE_ERROR("boot-menu-enable-with-timeout-invalid", NONE); + DO_TEST_PARSE_ERROR("boot-menu-enable-with-timeout-invalid", + QEMU_CAPS_SPLASH_TIMEOUT); DO_TEST("boot-menu-disable", NONE); DO_TEST("boot-menu-disable-drive", NONE); DO_TEST_PARSE_ERROR("boot-dev+order", -- 2.26.2
Move this check to virDomainDefVideoValidate() since it's not related to XML parsing. We don't have a failure test for this scenario, so a new test called 'video-multiple-primaries' was added to test this failure case. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 34 +++++++++++++++---- .../video-multiple-primaries.err | 1 + .../video-multiple-primaries.xml | 32 +++++++++++++++++ tests/qemuxml2argvtest.c | 5 +++ 4 files changed, 66 insertions(+), 6 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 e53a7372fc..4b5cab87e7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7321,6 +7321,30 @@ virDomainDefBootValidate(const virDomainDef *def) } +static int +virDomainDefVideoValidate(const virDomainDef *def) +{ + 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; + } + } + } + + return 0; +} + + static int virDomainDefValidateInternal(const virDomainDef *def, virDomainXMLOptionPtr xmlopt) @@ -7369,6 +7393,9 @@ virDomainDefValidateInternal(const virDomainDef *def, if (virDomainDefBootValidate(def) < 0) return -1; + if (virDomainDefVideoValidate(def) < 0) + return -1; + if (virDomainNumaDefValidate(def->numa) < 0) return -1; @@ -22162,14 +22189,9 @@ virDomainDefParseXML(xmlDocPtr xml, 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; - } - 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 202fc83ccf..0e7d8d5ba3 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2379,6 +2379,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
These checks are not related to XML parsing and can be moved to the validate callback. Errors were changed from VIR_ERR_XML_ERROR to VIR_ERR_CONFIG_UNSUPPORTED. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4b5cab87e7..bc09577e39 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6664,6 +6664,26 @@ virDomainVideoDefValidate(const virDomainVideoDef *video, return -1; } + 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; + } + } + return 0; } @@ -16234,11 +16254,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); @@ -16255,11 +16270,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); @@ -16268,11 +16278,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
The 'tray' check isn't a XML parse specific code and can be pushed to the validate callback, in virDomainDiskDefValidate(). 'vendor' and 'product' string sizes are already checked by the domaincommon.rng schema, but can be of use in the validate callback since not all scenarios will go through the XML parsing. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 47 ++++++++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 22 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bc09577e39..e9bafd189f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6115,6 +6115,9 @@ virSecurityDeviceLabelDefValidateXML(virSecurityDeviceLabelDefPtr *seclabels, } +#define VENDOR_LEN 8 +#define PRODUCT_LEN 16 + static int virDomainDiskDefValidate(const virDomainDef *def, const virDomainDiskDef *disk) @@ -6191,6 +6194,28 @@ virDomainDiskDefValidate(const virDomainDef *def, return -1; } + 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; + } + return 0; } @@ -10496,9 +10521,6 @@ virDomainDiskDefParsePrivateData(xmlXPathContextPtr ctxt, } -#define VENDOR_LEN 8 -#define PRODUCT_LEN 16 - static virDomainDiskDefPtr virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, xmlNodePtr node, @@ -10673,12 +10695,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")); @@ -10689,12 +10705,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")); @@ -10825,13 +10835,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, _("unknown disk tray status '%s'"), tray); return NULL; } - - if (def->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY && - def->device != VIR_DOMAIN_DISK_DEVICE_CDROM) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("tray is only valid for cdrom and floppy")); - return NULL; - } } if (removable) { -- 2.26.2
This check is not tied to XML parsing and can be moved to virDomainSmartcardDefValidate(). Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e9bafd189f..230e89e786 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6567,6 +6567,13 @@ static int virDomainSmartcardDefValidate(const virDomainSmartcardDef *smartcard, const virDomainDef *def) { + 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; + } + if (smartcard->type == VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH) return virDomainChrSourceDefValidate(smartcard->data.passthru, NULL, def); @@ -13682,13 +13689,6 @@ virDomainSmartcardDefParseXML(virDomainXMLOptionPtr xmlopt, if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, flags) < 0) return NULL; - 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")); - return NULL; - } - return g_steal_pointer(&def); } -- 2.26.2
Move this check to a new virDomainDefTunablesValidate(), which is called by virDomainDefValidateInternal(). Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 35 +++++++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 10 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 230e89e786..290930cc85 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7397,6 +7397,27 @@ virDomainDefVideoValidate(const virDomainDef *def) } +static int +virDomainDefTunablesValidate(const virDomainDef *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 virDomainDefValidateInternal(const virDomainDef *def, virDomainXMLOptionPtr xmlopt) @@ -7448,6 +7469,9 @@ virDomainDefValidateInternal(const virDomainDef *def, if (virDomainDefVideoValidate(def) < 0) return -1; + if (virDomainDefTunablesValidate(def) < 0) + return -1; + if (virDomainNumaDefValidate(def->numa) < 0) return -1; @@ -21342,7 +21366,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 */ @@ -21363,15 +21387,6 @@ virDomainDefTunablesParse(virDomainDefPtr def, &def->blkio.devices[i]) < 0) return -1; def->blkio.ndevices++; - for (j = 0; j < i; j++) { - if (STREQ(def->blkio.devices[j].path, - def->blkio.devices[i].path)) { - virReportError(VIR_ERR_XML_ERROR, - _("duplicate blkio device path '%s'"), - def->blkio.devices[i].path); - return -1; - } - } } VIR_FREE(nodes); -- 2.26.2
virDomainControllerDefParseXML() does a lot of checks with virDomainPCIControllerOpts parameters that can be moved to virDomainControllerDefValidate, sharing the logic with other use cases that does not rely on XML parsing. 'pseries-default-phb-numa-node' parse error was changed to reflect the error that is being thrown by qemuValidateDomainDeviceDefController() via deviceValidateCallback, that is executed before virDomainControllerDefValidate(). Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 89 ++++++++++--------- .../pseries-default-phb-numa-node.err | 2 +- tests/qemuxml2argvtest.c | 6 +- 3 files changed, 55 insertions(+), 42 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 290930cc85..8ede532e2a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6550,6 +6550,53 @@ virDomainControllerDefValidate(const virDomainControllerDef *controller) return -1; } } + + if (opts->chassisNr != -1) { + if (opts->chassisNr < 1 || opts->chassisNr > 255) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("PCI controller chassisNr '%d' out of range " + "- must be 1-255"), + opts->chassisNr); + return -1; + } + } + + if (opts->chassis != -1) { + if (opts->chassis < 0 || opts->chassis > 255) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("PCI controller chassis '%d' out of range " + "- must be 0-255"), + opts->chassis); + return -1; + } + } + + if (opts->port != -1) { + if (opts->port < 0 || opts->port > 255) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("PCI controller port '%d' out of range " + "- must be 0-255"), + opts->port); + return -1; + } + } + + if (opts->busNr != -1) { + if (opts->busNr < 1 || opts->busNr > 254) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("PCI controller busNr '%d' out of range " + "- must be 1-254"), + opts->busNr); + return -1; + } + } + + if (opts->numaNode >= 0 && controller->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; } @@ -11389,14 +11436,6 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr xmlopt, chassisNr); return NULL; } - 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); - return NULL; - } } if (chassis) { if (virStrToLong_i(chassis, NULL, 0, @@ -11406,14 +11445,6 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr xmlopt, chassis); return NULL; } - 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); - return NULL; - } } if (port) { if (virStrToLong_i(port, NULL, 0, @@ -11423,14 +11454,6 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr xmlopt, port); return NULL; } - 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); - return NULL; - } } if (busNr) { if (virStrToLong_i(busNr, NULL, 0, @@ -11440,14 +11463,6 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr xmlopt, busNr); return NULL; } - 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); - return NULL; - } } if (targetIndex) { if (virStrToLong_i(targetIndex, NULL, 0, @@ -11459,15 +11474,9 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr xmlopt, return NULL; } } - 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")); - return NULL; - } + if (numaNode >= 0) def->opts.pciopts.numaNode = numaNode; - } + if (hotplug) { int val = virTristateSwitchTypeFromString(hotplug); diff --git a/tests/qemuxml2argvdata/pseries-default-phb-numa-node.err b/tests/qemuxml2argvdata/pseries-default-phb-numa-node.err index 5d11109317..e46b710330 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: Option 'numaNode' is not valid for PCI controller with index '0', model 'pci-root' and modelName 'spapr-pci-host-bridge' diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 0e7d8d5ba3..9b853c6d59 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2115,7 +2115,11 @@ mymain(void) QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, QEMU_CAPS_SPAPR_PCI_HOST_BRIDGE_NUMA_NODE); - DO_TEST_PARSE_ERROR("pseries-default-phb-numa-node", NONE); + DO_TEST_PARSE_ERROR("pseries-default-phb-numa-node", + QEMU_CAPS_NUMA, + QEMU_CAPS_OBJECT_MEMORY_RAM, + QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, + QEMU_CAPS_SPAPR_PCI_HOST_BRIDGE_NUMA_NODE); DO_TEST_PARSE_ERROR("pseries-phb-invalid-target-index-1", NONE); DO_TEST_PARSE_ERROR("pseries-phb-invalid-target-index-2", NONE); DO_TEST_PARSE_ERROR("pseries-phb-invalid-target-index-3", NONE); -- 2.26.2
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 16 ++++++++++------ tests/qemuxml2argvdata/pci-root-address.err | 2 +- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8ede532e2a..11711ba05d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6524,6 +6524,16 @@ virDomainControllerDefValidate(const virDomainControllerDef *controller) if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { const virDomainPCIControllerOpts *opts = &controller->opts.pciopts; + if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT || + controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) { + if (controller->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 (controller->idx > 255) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("PCI controller index %d too high, maximum is 255"), @@ -11392,12 +11402,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")); - return NULL; - } 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
Create a new function called virDomainDefIdMapValidate() and use it to move these checks out of virDomainIdmapDefParseXML() and virDomainDefParseXML(). Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 42 ++++++++++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 11711ba05d..471de235a1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7475,6 +7475,29 @@ virDomainDefTunablesValidate(const virDomainDef *def) } +static int +virDomainDefIdMapValidate(const virDomainDef *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 virDomainDefValidateInternal(const virDomainDef *def, virDomainXMLOptionPtr xmlopt) @@ -7529,6 +7552,9 @@ virDomainDefValidateInternal(const virDomainDef *def, if (virDomainDefTunablesValidate(def) < 0) return -1; + if (virDomainDefIdMapValidate(def) < 0) + return -1; + if (virDomainNumaDefValidate(def->numa) < 0) return -1; @@ -19045,15 +19071,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; } @@ -22543,13 +22560,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
On 12/7/20 2:54 PM, Daniel Henrique Barboza wrote:
Hi,
This is the respin of [1] after the reviews from Michal. Although we're pushing code to validate callbacks instead of post parse functions, the idea and motivation is still in line with [2].
[1] https://www.redhat.com/archives/libvir-list/2020-November/msg01409.html [2] https://gitlab.com/libvirt/libvirt/-/issues/7
Daniel Henrique Barboza (9): domain_conf.c: move boot related timeouts check to validate callback domain_conf.c: move primary video check to validate callback domain_conf.c: move QXL attributes check to virDomainVideoDefValidate() domain_conf.c: move vendor, product and tray checks to validate callback domain_conf.c: move smartcard address check to validate callback domain_conf.c: move blkio path check to validate callback domain_conf.c: move virDomainPCIControllerOpts checks to validate callback domain_conf.c: move pci-root/pcie-root address check to validateCB domain_conf.c: move idmapEntry checks to validate callback
src/conf/domain_conf.c | 353 +++++++++++------- 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 | 14 +- 6 files changed, 268 insertions(+), 136 deletions(-) create mode 100644 tests/qemuxml2argvdata/video-multiple-primaries.err create mode 100644 tests/qemuxml2argvdata/video-multiple-primaries.xml
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> But since we are splitting parsing and validation code can we use this chance and move validators out of domain_conf.c allowing it to be smaller in size? Michal
On 12/8/20 7:22 AM, Michal Privoznik wrote:
On 12/7/20 2:54 PM, Daniel Henrique Barboza wrote:
Hi,
This is the respin of [1] after the reviews from Michal. Although we're pushing code to validate callbacks instead of post parse functions, the idea and motivation is still in line with [2].
[1] https://www.redhat.com/archives/libvir-list/2020-November/msg01409.html [2] https://gitlab.com/libvirt/libvirt/-/issues/7
Daniel Henrique Barboza (9): domain_conf.c: move boot related timeouts check to validate callback domain_conf.c: move primary video check to validate callback domain_conf.c: move QXL attributes check to virDomainVideoDefValidate() domain_conf.c: move vendor, product and tray checks to validate callback domain_conf.c: move smartcard address check to validate callback domain_conf.c: move blkio path check to validate callback domain_conf.c: move virDomainPCIControllerOpts checks to validate callback domain_conf.c: move pci-root/pcie-root address check to validateCB domain_conf.c: move idmapEntry checks to validate callback
src/conf/domain_conf.c | 353 +++++++++++------- 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 | 14 +- 6 files changed, 268 insertions(+), 136 deletions(-) create mode 100644 tests/qemuxml2argvdata/video-multiple-primaries.err create mode 100644 tests/qemuxml2argvdata/video-multiple-primaries.xml
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Thanks for the review!
But since we are splitting parsing and validation code can we use this chance and move validators out of domain_conf.c allowing it to be smaller in size?
Makes sense. I can create a 'domain_validate.c' and start moving stuff there, like I did previously with QEMU driver validations and qemu_validate.c. First order of business would be to move the validations I already played with in this series to this new file. What do you think? DHB
Michal
On 12/8/20 6:45 PM, Daniel Henrique Barboza wrote:
On 12/8/20 7:22 AM, Michal Privoznik wrote:
But since we are splitting parsing and validation code can we use this chance and move validators out of domain_conf.c allowing it to be smaller in size?
Makes sense. I can create a 'domain_validate.c' and start moving stuff there, like I did previously with QEMU driver validations and qemu_validate.c. First order of business would be to move the validations I already played with in this series to this new file.
What do you think?
Yes, qemu_validate.c example was what I had on mind. I don't have preference whether to move everything at once or by smaller pieces (how small?). While the latter might look like easier to review, not really since git diff has now --color-moved. That way a reviewer can verify that a commit is just moving code. Michal
On 12/8/20 4:58 PM, Michal Privoznik wrote:
On 12/8/20 6:45 PM, Daniel Henrique Barboza wrote:
On 12/8/20 7:22 AM, Michal Privoznik wrote:
But since we are splitting parsing and validation code can we use this chance and move validators out of domain_conf.c allowing it to be smaller in size?
Makes sense. I can create a 'domain_validate.c' and start moving stuff there, like I did previously with QEMU driver validations and qemu_validate.c. First order of business would be to move the validations I already played with in this series to this new file.
What do you think?
Yes, qemu_validate.c example was what I had on mind. I don't have preference whether to move everything at once or by smaller pieces (how small?). While the latter might look like easier to review, not really since git diff has now --color-moved. That way a reviewer can verify that a commit is just moving code.
In fact, I'm respining this series already creating domain_validate.c right from the start. I find this way more sensible than adding code to domain_conf.c just to move to a new file afterwards. I'll post a v3 shortly. I'll keep your R-bs in the patches you already reviewed in v2 (the changes aren't worth reviewing it again). About how we're going to move the validations, I'll do what I did with qemu_validate.c. For example, move all static functions that is called by virDomainDefValidateInternal() in a single punch, then move virDomainDefValidateInternal(). Same thing with other relevant validation functions. This can be done in a follow-up series after this one. DHB
Michal
participants (2)
-
Daniel Henrique Barboza -
Michal Privoznik