7:54 a.m.
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
7:54 a.m.
New subject: [PATCH v2 1/9] domain_conf.c: move boot related timeouts check to validate callback
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(a)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
7:54 a.m.
New subject: [PATCH v2 2/9] domain_conf.c: move primary video check to validate callback
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(a)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
7:54 a.m.
New subject: [PATCH v2 3/9] domain_conf.c: move QXL attributes check to virDomainVideoDefValidate()
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(a)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
7:54 a.m.
New subject: [PATCH v2 4/9] domain_conf.c: move vendor, product and tray checks to validate callback
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(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 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
7:54 a.m.
New subject: [PATCH v2 5/9] domain_conf.c: move smartcard address check to validate callback
This check is not tied to XML parsing and can be moved to
virDomainSmartcardDefValidate().
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)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
7:54 a.m.
New subject: [PATCH v2 6/9] domain_conf.c: move blkio path check to validate callback
Move this check to a new virDomainDefTunablesValidate(), which
is called by virDomainDefValidateInternal().
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 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
7:54 a.m.
New subject: [PATCH v2 7/9] domain_conf.c: move virDomainPCIControllerOpts checks to validate callback
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(a)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
7:54 a.m.
New subject: [PATCH v2 8/9] domain_conf.c: move pci-root/pcie-root address check to validateCB
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 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
7:54 a.m.
New subject: [PATCH v2 9/9] domain_conf.c: move idmapEntry checks to validate callback
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(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 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
4:22 a.m.
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(a)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
11:45 a.m.
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(a)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
1:58 p.m.
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
2:45 p.m.
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
1445
days inactive
1446
days old
13 comments
2 participants
participants (2)
-
Daniel Henrique Barboza -
Michal Privoznik