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@xxxxxxxxxx>
> ---
> 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;
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list