Re: [libvirt] [PATCH v2 1/7] domain_conf: Validate redirdev after parsing
On 06/10/2016 11:32 AM, Michal Privoznik wrote:
There's currently just one limitation: redirdevs that want to go
on USB bus require a USB controller, surprisingly.
At the same time, since I'm using virDomainDefHasUSB() in this
new validator function, it has to be moved a few lines up and
also its header needed to be changed a bit: it is now taking a
const pointer to domain def since it's not changing anything in
there.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 56 ++++++++++++++++++++++++++++----------------------
1 file changed, 32 insertions(+), 24 deletions(-)
The commit message makes me feel like I missed part of the story at
least with the way it starts out...
ACK (although some may say this could have been two patches - one to
move the function and one to add the virDomainRedirdevDefValidate)
John
I made two nit notes below... Not anything that would hold this up.
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 85f6e31..c75279d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4567,14 +4567,45 @@ virDomainDiskDefValidate(const virDomainDiskDef *disk)
return 0;
}
+static bool
+virDomainDefHasUSB(const virDomainDef *def)
+{
+ size_t i;
+
+ for (i = 0; i < def->ncontrollers; i++) {
+ if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_USB
&&
+ def->controllers[i]->model != VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE)
+ return true;
+ }
+
+ return false;
+}
+
+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",
+ _("Can't add redirected USB device: "
+ "USB is disabled for this domain"));
Sigh - Can't... I'm probably just as guilty though... We ought to have a
large bite size task to remove all contractions such as this... and
maybe another to standardize whether we start w/ capital letter or
lowercase.
+ return -1;
+ }
+
+ return 0;
+}
+
static int
virDomainDeviceDefValidateInternal(const virDomainDeviceDef *dev,
- const virDomainDef *def ATTRIBUTE_UNUSED)
+ const virDomainDef *def)
{
switch ((virDomainDeviceType) dev->type) {
case VIR_DOMAIN_DEVICE_DISK:
return virDomainDiskDefValidate(dev->data.disk);
+ case VIR_DOMAIN_DEVICE_REDIRDEV:
+ return virDomainRedirdevDefValidate(def, dev->data.redirdev);
NIT: my eyes, my eyes... Nice to have extra lines between sometimes!
case VIR_DOMAIN_DEVICE_LEASE:
case VIR_DOMAIN_DEVICE_FS:
case VIR_DOMAIN_DEVICE_NET:
@@ -4586,7 +4617,6 @@ virDomainDeviceDefValidateInternal(const virDomainDeviceDef *dev,
case VIR_DOMAIN_DEVICE_CONTROLLER:
case VIR_DOMAIN_DEVICE_GRAPHICS:
case VIR_DOMAIN_DEVICE_HUB:
- case VIR_DOMAIN_DEVICE_REDIRDEV:
case VIR_DOMAIN_DEVICE_SMARTCARD:
case VIR_DOMAIN_DEVICE_CHR:
case VIR_DOMAIN_DEVICE_MEMBALLOON:
@@ -17060,14 +17090,6 @@ virDomainDefParseXML(xmlDocPtr xml,
if (!redirdev)
goto error;
- if (redirdev->bus == VIR_DOMAIN_REDIRDEV_BUS_USB && usb_none) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Can't add redirected USB device: "
- "USB is disabled for this domain"));
- virDomainRedirdevDefFree(redirdev);
- goto error;
- }
-
def->redirdevs[def->nredirdevs++] = redirdev;
}
VIR_FREE(nodes);
@@ -23578,20 +23600,6 @@ virDomainObjFormat(virDomainXMLOptionPtr xmlopt,
}
static bool
-virDomainDefHasUSB(virDomainDefPtr def)
-{
- size_t i;
-
- for (i = 0; i < def->ncontrollers; i++) {
- if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_USB
&&
- def->controllers[i]->model != VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE)
- return true;
- }
-
- return false;
-}
-
-static bool
virDomainDeviceIsUSB(virDomainDeviceDefPtr dev)
{
int t = dev->type;