On 12/8/20 11:20 PM, Daniel Henrique Barboza wrote: Honestly, I have only vague idea how these Copyright lines work, but shouldn't they also include (at least subset) of the lines from the original file? I mean, my common sense tells me that if I have a file written by person X, and later the file is split into two the person X is still the original author. Extending that, if a company holds a copyright on a file then moving bits out to a different file should keep the copyright. But I admit that law has completely different model of "common sense". And also there is a disconnection between files and these Copyright lines. If a copyright holder Y changed a tiny bit that is not moved - should their Copyright line also appear in the new file? Michal

On Wed, Dec 09, 2020 at 07:35:09AM -0300, Daniel Henrique Barboza wrote: If you're copying code from one file to a new file, then the simplest way is to copy the existing copyright headers unchanged. Not copying a copyright header would require you to audit the history of the code to determine whether it is valid to exclude them. This is usually a waste of time, so preserving the full existing copyright headers is normal practice for sake of speed. You can then optionally also add new copyright lines if you're also making code changes after the copy. A combination of your country's laws, and your employer's rules generally determine whether copyright statements have to be in your name or your employer's name. Most commonly it is your employer's name, as most companies require their employees to assign copyright on all their work as part of terms of employment. There are exceptions though, so I can't explicitly say what is right for you personally. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

This check isn't exclusive to XML parsing. Let's move it to virDomainDefVideoValidate() in domain_validate.c 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. Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com&gt; Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; --- src/conf/domain_conf.c | 10 +++--- src/conf/domain_validate.c | 24 ++++++++++++++ src/conf/domain_validate.h | 1 + .../video-multiple-primaries.err | 1 + .../video-multiple-primaries.xml | 32 +++++++++++++++++++ tests/qemuxml2argvtest.c | 5 +++ 6 files changed, 67 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 4a45c76cf1..a8bd54a368 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7348,6 +7348,9 @@ virDomainDefValidateInternal(const virDomainDef *def, if (virDomainDefBootValidate(def) < 0) return -1; + if (virDomainDefVideoValidate(def) < 0) + return -1; + if (virDomainNumaDefValidate(def->numa) < 0) return -1; @@ -22141,14 +22144,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/src/conf/domain_validate.c b/src/conf/domain_validate.c index e4d947c553..eb2ef6c7fb 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c @@ -49,3 +49,27 @@ virDomainDefBootValidate(const virDomainDef *def) return 0; } + + +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; +} diff --git a/src/conf/domain_validate.h b/src/conf/domain_validate.h index 2b558668a9..df4f35c1dd 100644 --- a/src/conf/domain_validate.h +++ b/src/conf/domain_validate.h @@ -25,3 +25,4 @@ #include "domain_conf.h" int virDomainDefBootValidate(const virDomainDef *def); +int virDomainDefVideoValidate(const virDomainDef *def); 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. Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com&gt; Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; --- src/conf/domain_conf.c | 15 --------------- src/conf/domain_validate.c | 20 ++++++++++++++++++++ 2 files changed, 20 insertions(+), 15 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d80bc8495e..db0ca975fe 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16132,11 +16132,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); @@ -16153,11 +16148,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); @@ -16166,11 +16156,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); diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c index c81fd296b9..234eb72f11 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c @@ -128,5 +128,25 @@ 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; } -- 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. Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com&gt; Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; --- src/conf/domain_conf.c | 22 ---------------------- src/conf/domain_validate.c | 25 +++++++++++++++++++++++++ 2 files changed, 25 insertions(+), 22 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5aeb75ce59..220bb8cdcf 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10222,9 +10222,6 @@ virDomainDiskDefParsePrivateData(xmlXPathContextPtr ctxt, } -#define VENDOR_LEN 8 -#define PRODUCT_LEN 16 - static virDomainDiskDefPtr virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, xmlNodePtr node, @@ -10399,12 +10396,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")); @@ -10415,12 +10406,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")); @@ -10551,13 +10536,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) { diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c index da36bef31a..bc5ca3f668 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c @@ -225,6 +225,9 @@ virSecurityDeviceLabelDefValidateXML(virSecurityDeviceLabelDefPtr *seclabels, } +#define VENDOR_LEN 8 +#define PRODUCT_LEN 16 + int virDomainDiskDefValidate(const virDomainDef *def, const virDomainDiskDef *disk) @@ -301,5 +304,27 @@ 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; } -- 2.26.2

Next patch will move a validation to virDomainSmartcardDefValidate(), but this function can't be moved alone to domain_validate.c without making virDomainChrSourceDefValidate(), from domain_conf.c, public. Given that the idea is to eventually move all validations to domain_validate.c anyways, let's move all ChrSource related validations in a single punch. Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; --- src/conf/domain_conf.c | 161 ------------------------------------- src/conf/domain_validate.c | 161 +++++++++++++++++++++++++++++++++++++ src/conf/domain_validate.h | 8 ++ 3 files changed, 169 insertions(+), 161 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6578055119..a301ab5c74 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6058,137 +6058,6 @@ virDomainDefHasUSB(const virDomainDef *def) } -#define SERIAL_CHANNEL_NAME_CHARS \ - "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_-." - - -static int -virDomainChrSourceDefValidate(const virDomainChrSourceDef *src_def, - const virDomainChrDef *chr_def, - const virDomainDef *def) -{ - switch ((virDomainChrType) src_def->type) { - case VIR_DOMAIN_CHR_TYPE_NULL: - case VIR_DOMAIN_CHR_TYPE_PTY: - case VIR_DOMAIN_CHR_TYPE_VC: - case VIR_DOMAIN_CHR_TYPE_STDIO: - case VIR_DOMAIN_CHR_TYPE_SPICEVMC: - case VIR_DOMAIN_CHR_TYPE_LAST: - break; - - case VIR_DOMAIN_CHR_TYPE_FILE: - case VIR_DOMAIN_CHR_TYPE_DEV: - case VIR_DOMAIN_CHR_TYPE_PIPE: - if (!src_def->data.file.path) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Missing source path attribute for char device")); - return -1; - } - break; - - case VIR_DOMAIN_CHR_TYPE_NMDM: - if (!src_def->data.nmdm.master) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Missing master path attribute for nmdm device")); - return -1; - } - - if (!src_def->data.nmdm.slave) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Missing slave path attribute for nmdm device")); - return -1; - } - break; - - case VIR_DOMAIN_CHR_TYPE_TCP: - if (!src_def->data.tcp.host) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Missing source host attribute for char device")); - return -1; - } - - if (!src_def->data.tcp.service) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Missing source service attribute for char device")); - return -1; - } - - if (src_def->data.tcp.listen && src_def->data.tcp.reconnect.enabled) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("chardev reconnect is possible only for connect mode")); - return -1; - } - break; - - case VIR_DOMAIN_CHR_TYPE_UDP: - if (!src_def->data.udp.connectService) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Missing source service attribute for char device")); - return -1; - } - break; - - case VIR_DOMAIN_CHR_TYPE_UNIX: - /* The source path can be auto generated for certain specific - * types of channels, but in most cases we should report an - * error if the user didn't provide it */ - if (!src_def->data.nix.path && - !(chr_def && - chr_def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL && - (chr_def->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN || - chr_def->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Missing source path attribute for char device")); - return -1; - } - - if (src_def->data.nix.listen && src_def->data.nix.reconnect.enabled) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("chardev reconnect is possible only for connect mode")); - return -1; - } - break; - - case VIR_DOMAIN_CHR_TYPE_SPICEPORT: - if (!src_def->data.spiceport.channel) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Missing source channel attribute for char device")); - return -1; - } - if (strspn(src_def->data.spiceport.channel, - SERIAL_CHANNEL_NAME_CHARS) < strlen(src_def->data.spiceport.channel)) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("Invalid character in source channel for char device")); - return -1; - } - break; - } - - if (virSecurityDeviceLabelDefValidate(src_def->seclabels, - src_def->nseclabels, - def->seclabels, - def->nseclabels) < 0) - return -1; - - return 0; -} - - -static int -virDomainRedirdevDefValidate(const virDomainDef *def, - const virDomainRedirdevDef *redirdev) -{ - if (redirdev->bus == VIR_DOMAIN_REDIRDEV_BUS_USB && - !virDomainDefHasUSB(def)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("cannot add redirected USB device: " - "USB is disabled for this domain")); - return -1; - } - - return virDomainChrSourceDefValidate(redirdev->source, NULL, def); -} - static int virDomainNetDefValidatePortOptions(const char *macstr, @@ -6379,36 +6248,6 @@ virDomainControllerDefValidate(const virDomainControllerDef *controller) } -static int -virDomainChrDefValidate(const virDomainChrDef *chr, - const virDomainDef *def) -{ - return virDomainChrSourceDefValidate(chr->source, chr, def); -} - - -static int -virDomainSmartcardDefValidate(const virDomainSmartcardDef *smartcard, - const virDomainDef *def) -{ - if (smartcard->type == VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH) - return virDomainChrSourceDefValidate(smartcard->data.passthru, NULL, def); - - return 0; -} - - -static int -virDomainRNGDefValidate(const virDomainRNGDef *rng, - const virDomainDef *def) -{ - if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD) - return virDomainChrSourceDefValidate(rng->source.chardev, NULL, def); - - return 0; -} - - static int virDomainHostdevDefValidate(const virDomainHostdevDef *hostdev) { diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c index e9427cffa8..882fbdac57 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c @@ -328,3 +328,164 @@ virDomainDiskDefValidate(const virDomainDef *def, return 0; } + + +#define SERIAL_CHANNEL_NAME_CHARS \ + "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_-." + +static int +virDomainChrSourceDefValidate(const virDomainChrSourceDef *src_def, + const virDomainChrDef *chr_def, + const virDomainDef *def) +{ + switch ((virDomainChrType) src_def->type) { + case VIR_DOMAIN_CHR_TYPE_NULL: + case VIR_DOMAIN_CHR_TYPE_PTY: + case VIR_DOMAIN_CHR_TYPE_VC: + case VIR_DOMAIN_CHR_TYPE_STDIO: + case VIR_DOMAIN_CHR_TYPE_SPICEVMC: + case VIR_DOMAIN_CHR_TYPE_LAST: + break; + + case VIR_DOMAIN_CHR_TYPE_FILE: + case VIR_DOMAIN_CHR_TYPE_DEV: + case VIR_DOMAIN_CHR_TYPE_PIPE: + if (!src_def->data.file.path) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing source path attribute for char device")); + return -1; + } + break; + + case VIR_DOMAIN_CHR_TYPE_NMDM: + if (!src_def->data.nmdm.master) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing master path attribute for nmdm device")); + return -1; + } + + if (!src_def->data.nmdm.slave) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing slave path attribute for nmdm device")); + return -1; + } + break; + + case VIR_DOMAIN_CHR_TYPE_TCP: + if (!src_def->data.tcp.host) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing source host attribute for char device")); + return -1; + } + + if (!src_def->data.tcp.service) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing source service attribute for char device")); + return -1; + } + + if (src_def->data.tcp.listen && src_def->data.tcp.reconnect.enabled) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("chardev reconnect is possible only for connect mode")); + return -1; + } + break; + + case VIR_DOMAIN_CHR_TYPE_UDP: + if (!src_def->data.udp.connectService) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing source service attribute for char device")); + return -1; + } + break; + + case VIR_DOMAIN_CHR_TYPE_UNIX: + /* The source path can be auto generated for certain specific + * types of channels, but in most cases we should report an + * error if the user didn't provide it */ + if (!src_def->data.nix.path && + !(chr_def && + chr_def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL && + (chr_def->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN || + chr_def->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing source path attribute for char device")); + return -1; + } + + if (src_def->data.nix.listen && src_def->data.nix.reconnect.enabled) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("chardev reconnect is possible only for connect mode")); + return -1; + } + break; + + case VIR_DOMAIN_CHR_TYPE_SPICEPORT: + if (!src_def->data.spiceport.channel) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Missing source channel attribute for char device")); + return -1; + } + if (strspn(src_def->data.spiceport.channel, + SERIAL_CHANNEL_NAME_CHARS) < strlen(src_def->data.spiceport.channel)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("Invalid character in source channel for char device")); + return -1; + } + break; + } + + if (virSecurityDeviceLabelDefValidate(src_def->seclabels, + src_def->nseclabels, + def->seclabels, + def->nseclabels) < 0) + return -1; + + return 0; +} + + +int +virDomainRedirdevDefValidate(const virDomainDef *def, + const virDomainRedirdevDef *redirdev) +{ + if (redirdev->bus == VIR_DOMAIN_REDIRDEV_BUS_USB && + !virDomainDefHasUSB(def)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("cannot add redirected USB device: " + "USB is disabled for this domain")); + return -1; + } + + return virDomainChrSourceDefValidate(redirdev->source, NULL, def); +} + + +int +virDomainChrDefValidate(const virDomainChrDef *chr, + const virDomainDef *def) +{ + return virDomainChrSourceDefValidate(chr->source, chr, def); +} + + +int +virDomainRNGDefValidate(const virDomainRNGDef *rng, + const virDomainDef *def) +{ + if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD) + return virDomainChrSourceDefValidate(rng->source.chardev, NULL, def); + + return 0; +} + + +int +virDomainSmartcardDefValidate(const virDomainSmartcardDef *smartcard, + const virDomainDef *def) +{ + if (smartcard->type == VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH) + return virDomainChrSourceDefValidate(smartcard->data.passthru, NULL, def); + + return 0; +} diff --git a/src/conf/domain_validate.h b/src/conf/domain_validate.h index aef169a4c9..d65de50422 100644 --- a/src/conf/domain_validate.h +++ b/src/conf/domain_validate.h @@ -34,3 +34,11 @@ int virSecurityDeviceLabelDefValidate(virSecurityDeviceLabelDefPtr *seclabels, size_t nvmSeclabels); int virDomainDiskDefValidate(const virDomainDef *def, const virDomainDiskDef *disk); +int virDomainRedirdevDefValidate(const virDomainDef *def, + const virDomainRedirdevDef *redirdev); +int virDomainChrDefValidate(const virDomainChrDef *chr, + const virDomainDef *def); +int virDomainRNGDefValidate(const virDomainRNGDef *rng, + const virDomainDef *def); +int virDomainSmartcardDefValidate(const virDomainSmartcardDef *smartcard, + const virDomainDef *def); -- 2.26.2

Move this check to a new virDomainDefTunablesValidate(), which is called by virDomainDefValidateInternal(). Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com&gt; Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; --- src/conf/domain_conf.c | 14 ++++---------- src/conf/domain_validate.c | 21 +++++++++++++++++++++ src/conf/domain_validate.h | 1 + 3 files changed, 26 insertions(+), 10 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a5dcb0bbce..064d77d933 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6981,6 +6981,9 @@ virDomainDefValidateInternal(const virDomainDef *def, if (virDomainDefVideoValidate(def) < 0) return -1; + if (virDomainDefTunablesValidate(def) < 0) + return -1; + if (virDomainNumaDefValidate(def->numa) < 0) return -1; @@ -20875,7 +20878,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 */ @@ -20896,15 +20899,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); diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c index 6fca604d17..09ab908ea3 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c @@ -496,3 +496,24 @@ virDomainSmartcardDefValidate(const virDomainSmartcardDef *smartcard, return 0; } + + +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; +} diff --git a/src/conf/domain_validate.h b/src/conf/domain_validate.h index d65de50422..2bd9e71073 100644 --- a/src/conf/domain_validate.h +++ b/src/conf/domain_validate.h @@ -42,3 +42,4 @@ int virDomainRNGDefValidate(const virDomainRNGDef *rng, const virDomainDef *def); int virDomainSmartcardDefValidate(const virDomainSmartcardDef *smartcard, const virDomainDef *def); +int virDomainDefTunablesValidate(const virDomainDef *def); -- 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(). Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com&gt; Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; --- src/conf/domain_conf.c | 42 +--------------- src/conf/domain_validate.c | 48 +++++++++++++++++++ .../pseries-default-phb-numa-node.err | 2 +- tests/qemuxml2argvtest.c | 6 ++- 4 files changed, 56 insertions(+), 42 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 453dc6cf6a..7adf2700ae 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10864,14 +10864,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, @@ -10881,14 +10873,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, @@ -10898,14 +10882,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, @@ -10915,14 +10891,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, @@ -10934,15 +10902,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/src/conf/domain_validate.c b/src/conf/domain_validate.c index 416c24f97b..f47e80ca38 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c @@ -551,6 +551,54 @@ 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; } 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

Create a new function called virDomainDefIdMapValidate() and use it to move these checks out of virDomainIdmapDefParseXML() and virDomainDefParseXML(). Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com&gt; Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; --- src/conf/domain_conf.c | 19 +++---------------- src/conf/domain_validate.c | 23 +++++++++++++++++++++++ src/conf/domain_validate.h | 1 + 3 files changed, 27 insertions(+), 16 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7a7c9ec85c..44f136f530 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6947,6 +6947,9 @@ virDomainDefValidateInternal(const virDomainDef *def, if (virDomainDefTunablesValidate(def) < 0) return -1; + if (virDomainDefIdMapValidate(def) < 0) + return -1; + if (virDomainNumaDefValidate(def->numa) < 0) return -1; @@ -18463,15 +18466,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; } @@ -21961,13 +21955,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; diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c index 35deb9f017..0eed1ba982 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c @@ -612,3 +612,26 @@ virDomainControllerDefValidate(const virDomainControllerDef *controller) return 0; } + + +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; +} diff --git a/src/conf/domain_validate.h b/src/conf/domain_validate.h index e8004e358d..497a02b9b3 100644 --- a/src/conf/domain_validate.h +++ b/src/conf/domain_validate.h @@ -44,3 +44,4 @@ int virDomainSmartcardDefValidate(const virDomainSmartcardDef *smartcard, const virDomainDef *def); int virDomainDefTunablesValidate(const virDomainDef *def); int virDomainControllerDefValidate(const virDomainControllerDef *controller); +int virDomainDefIdMapValidate(const virDomainDef *def); -- 2.26.2