After a successful attach, the device address has already been set. Remove the pointless assignment. --- src/qemu/qemu_hotplug.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 0288986d8..2d13c2891 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -590,11 +590,8 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver, goto cleanup; } - if (ret == 0) { - if (controller->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) - controller->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; + if (ret == 0) virDomainControllerInsertPreAlloced(vm->def, controller); - } cleanup: if (ret != 0 && releaseaddr) -- 2.13.0

Let it be reused in qemu_domain_address. --- src/qemu/qemu_command.c | 40 ---------------------------------------- src/qemu/qemu_command.h | 5 ----- src/qemu/qemu_domain.c | 40 ++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 5 +++++ 4 files changed, 45 insertions(+), 45 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f68b82d08..138bbdf1a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1263,46 +1263,6 @@ qemuCheckDiskConfig(virDomainDiskDefPtr disk) } -/* Check whether the device address is using either 'ccw' or default s390 - * address format and whether that's "legal" for the current qemu and/or - * guest os.machine type. This is the corollary to the code which doesn't - * find the address type set using an emulator that supports either 'ccw' - * or s390 and sets the address type based on the capabilities. - * - * If the address is using 'ccw' or s390 and it's not supported, generate - * an error and return false; otherwise, return true. - */ -bool -qemuCheckCCWS390AddressSupport(const virDomainDef *def, - virDomainDeviceInfo info, - virQEMUCapsPtr qemuCaps, - const char *devicename) -{ - if (info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { - if (!qemuDomainIsS390CCW(def)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("cannot use CCW address type for device " - "'%s' using machine type '%s'"), - devicename, def->os.machine); - return false; - } else if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_CCW)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("CCW address type is not supported by " - "this QEMU")); - return false; - } - } else if (info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390) { - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_S390)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("virtio S390 address type is not supported by " - "this QEMU")); - return false; - } - } - return true; -} - - /* QEMU 1.2 and later have a binary flag -enable-fips that must be * used for VNC auth to obey FIPS settings; but the flag only * exists on Linux, and with no way to probe for it via QMP. Our diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 94e4592cc..1254ad4df 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -189,11 +189,6 @@ int qemuCheckDiskConfig(virDomainDiskDefPtr disk); bool qemuCheckFips(void); -bool qemuCheckCCWS390AddressSupport(const virDomainDef *def, - virDomainDeviceInfo info, - virQEMUCapsPtr qemuCaps, - const char *devicename); - virJSONValuePtr qemuBuildHotpluggableCPUProps(const virDomainVcpuDef *vcpu) ATTRIBUTE_NONNULL(1); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 05e8b96aa..f6eb4277a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -10122,3 +10122,43 @@ qemuDomainGetMachineName(virDomainObjPtr vm) return ret; } + + +/* Check whether the device address is using either 'ccw' or default s390 + * address format and whether that's "legal" for the current qemu and/or + * guest os.machine type. This is the corollary to the code which doesn't + * find the address type set using an emulator that supports either 'ccw' + * or s390 and sets the address type based on the capabilities. + * + * If the address is using 'ccw' or s390 and it's not supported, generate + * an error and return false; otherwise, return true. + */ +bool +qemuCheckCCWS390AddressSupport(const virDomainDef *def, + virDomainDeviceInfo info, + virQEMUCapsPtr qemuCaps, + const char *devicename) +{ + if (info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { + if (!qemuDomainIsS390CCW(def)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("cannot use CCW address type for device " + "'%s' using machine type '%s'"), + devicename, def->os.machine); + return false; + } else if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_CCW)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("CCW address type is not supported by " + "this QEMU")); + return false; + } + } else if (info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_S390)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("virtio S390 address type is not supported by " + "this QEMU")); + return false; + } + } + return true; +} diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 5201c6a0a..6abefc929 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -978,4 +978,9 @@ qemuDomainFixupCPUs(virDomainObjPtr vm, char * qemuDomainGetMachineName(virDomainObjPtr vm); +bool qemuCheckCCWS390AddressSupport(const virDomainDef *def, + virDomainDeviceInfo info, + virQEMUCapsPtr qemuCaps, + const char *devicename); + #endif /* __QEMU_DOMAIN_H__ */ -- 2.13.0

On Thu, Oct 19, 2017 at 08:08:34AM -0400, John Ferlan wrote: The function does not deal directly with the command line and including qemu_command.h would feel wrong. [...] I purposefully formatted it this way to match most of the prototypes in the file. Jan

On 10/17/2017 11:04 AM, Ján Tomko wrote: Not an issue - just a note from review... This is only setting *releaseAddr in this instance; whereas, the previous code would also set it for info->type CCW [1] Still looking at how @releaseaddr is used, we see that it's used to call qemuDomainReleaseDeviceAddress which only cares about PCI and USB, so I suppose this is fine - just different. [1] @releaseaddr is set for both CCW and PCI here.

https://bugzilla.redhat.com/show_bug.cgi?id=1379603 --- src/qemu/qemu_driver.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index fb4d72236..050a41524 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8060,6 +8060,10 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef, break; case VIR_DOMAIN_DEVICE_INPUT: + if (VIR_APPEND_ELEMENT(vmdef->inputs, vmdef->ninputs, dev->data.input) < 0) + return -1; + break; + case VIR_DOMAIN_DEVICE_SOUND: case VIR_DOMAIN_DEVICE_VIDEO: case VIR_DOMAIN_DEVICE_GRAPHICS: -- 2.13.0

Move assignment of input device alias into a separate function, for reuse on hotplug. --- src/qemu/qemu_alias.c | 24 +++++++++++++++++++++++- src/qemu/qemu_alias.h | 3 +++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index 72df1083f..737fc2fda 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -412,6 +412,28 @@ qemuAssignDeviceWatchdogAlias(virDomainWatchdogDefPtr watchdog) if (VIR_STRDUP(watchdog->info.alias, "watchdog0") < 0) return -1; + + return 0; +} + +int +qemuAssignDeviceInputAlias(virDomainDefPtr def, + virDomainInputDefPtr input, + int idx) +{ + if (idx == -1) { + int thisidx; + size_t i; + + for (i = 0; i < def->ninputs; i++) { + if ((thisidx = qemuDomainDeviceAliasIndex(&def->inputs[i]->info, "input")) >= idx) + idx = thisidx + 1; + } + } + + if (virAsprintf(&input->info.alias, "input%d", idx) < 0) + return -1; + return 0; } @@ -461,7 +483,7 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) return -1; } for (i = 0; i < def->ninputs; i++) { - if (virAsprintf(&def->inputs[i]->info.alias, "input%zu", i) < 0) + if (qemuAssignDeviceInputAlias(def, def->inputs[i], i) < 0) return -1; } for (i = 0; i < def->nparallels; i++) { diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h index 652ffea0c..35424829f 100644 --- a/src/qemu/qemu_alias.h +++ b/src/qemu/qemu_alias.h @@ -66,6 +66,9 @@ int qemuAssignDeviceShmemAlias(virDomainDefPtr def, int idx); int qemuAssignDeviceWatchdogAlias(virDomainWatchdogDefPtr watchdog); +int qemuAssignDeviceInputAlias(virDomainDefPtr def, + virDomainInputDefPtr input, + int idx); int qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps); -- 2.13.0

A function that builds the -device string for input devices. --- src/qemu/qemu_command.c | 42 +++++++++++++++++++++++++++++------------- src/qemu/qemu_command.h | 7 +++++++ 2 files changed, 36 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 138bbdf1a..7bdff85fc 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4303,6 +4303,27 @@ qemuBuildUSBInputDevStr(const virDomainDef *def, } +int +qemuBuildInputDevStr(char **devstr, + const virDomainDef *def, + virDomainInputDefPtr input, + virQEMUCapsPtr qemuCaps) +{ + switch (input->bus) { + case VIR_DOMAIN_INPUT_BUS_USB: + if (!(*devstr = qemuBuildUSBInputDevStr(def, input, qemuCaps))) + return -1; + break; + + case VIR_DOMAIN_INPUT_BUS_VIRTIO: + if (!(*devstr = qemuBuildVirtioInputDevStr(def, input, qemuCaps))) + return -1; + break; + } + return 0; +} + + static int qemuBuildInputCommandLine(virCommandPtr cmd, const virDomainDef *def, @@ -4312,22 +4333,17 @@ qemuBuildInputCommandLine(virCommandPtr cmd, for (i = 0; i < def->ninputs; i++) { virDomainInputDefPtr input = def->inputs[i]; + char *devstr = NULL; - if (input->bus == VIR_DOMAIN_INPUT_BUS_USB) { - char *optstr; - virCommandAddArg(cmd, "-device"); - if (!(optstr = qemuBuildUSBInputDevStr(def, input, qemuCaps))) - return -1; - virCommandAddArg(cmd, optstr); - VIR_FREE(optstr); - } else if (input->bus == VIR_DOMAIN_INPUT_BUS_VIRTIO) { - char *optstr; + if (qemuBuildInputDevStr(&devstr, def, input, qemuCaps) < 0) + return -1; + + if (devstr) { virCommandAddArg(cmd, "-device"); - if (!(optstr = qemuBuildVirtioInputDevStr(def, input, qemuCaps))) - return -1; - virCommandAddArg(cmd, optstr); - VIR_FREE(optstr); + virCommandAddArg(cmd, devstr); } + + VIR_FREE(devstr); } return 0; diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 1254ad4df..0961ec8cb 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -204,4 +204,11 @@ char *qemuBuildWatchdogDevStr(const virDomainDef *def, virDomainWatchdogDefPtr dev, virQEMUCapsPtr qemuCaps); +int qemuBuildInputDevStr(char **devstr, + const virDomainDef *def, + virDomainInputDefPtr input, + virQEMUCapsPtr qemuCaps) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) + ATTRIBUTE_NONNULL(4); + #endif /* __QEMU_COMMAND_H__*/ -- 2.13.0

For both virtio input devices and USB input devices. https://bugzilla.redhat.com/show_bug.cgi?id=1379603 --- src/libvirt_private.syms | 2 ++ src/qemu/qemu_driver.c | 9 +++++- src/qemu/qemu_hotplug.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_hotplug.h | 5 ++++ 4 files changed, 88 insertions(+), 1 deletion(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7713ecc01..f5cdbeb66 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -157,6 +157,7 @@ virDomainAuditDisk; virDomainAuditFS; virDomainAuditHostdev; virDomainAuditInit; +virDomainAuditInput; virDomainAuditIOThread; virDomainAuditMemory; virDomainAuditNet; @@ -385,6 +386,7 @@ virDomainHubTypeFromString; virDomainHubTypeToString; virDomainHypervTypeFromString; virDomainHypervTypeToString; +virDomainInputBusTypeToString; virDomainInputDefFind; virDomainInputDefFree; virDomainIOMMUModelTypeFromString; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 092df673c..75a0e42aa 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7664,9 +7664,16 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm, } break; + case VIR_DOMAIN_DEVICE_INPUT: + ret = qemuDomainAttachInputDevice(driver, vm, dev->data.input); + if (ret == 0) { + alias = dev->data.input->info.alias; + dev->data.input = NULL; + } + break; + case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_FS: - case VIR_DOMAIN_DEVICE_INPUT: case VIR_DOMAIN_DEVICE_SOUND: case VIR_DOMAIN_DEVICE_VIDEO: case VIR_DOMAIN_DEVICE_GRAPHICS: diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index c65e7e500..b32acb71e 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2905,6 +2905,79 @@ qemuDomainAttachWatchdog(virQEMUDriverPtr driver, } +int +qemuDomainAttachInputDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainInputDefPtr input) +{ + int ret = -1; + char *devstr = NULL; + qemuDomainObjPrivatePtr priv = vm->privateData; + virDomainDeviceDef dev = { VIR_DOMAIN_DEVICE_INPUT, + { .input = input } }; + bool releaseaddr = false; + + if (input->bus != VIR_DOMAIN_INPUT_BUS_USB && + input->bus != VIR_DOMAIN_INPUT_BUS_VIRTIO) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("input device on bus '%s' cannot be hot plugged."), + virDomainInputBusTypeToString(input->bus)); + return -1; + } + + if (input->bus == VIR_DOMAIN_INPUT_BUS_VIRTIO) { + if (qemuDomainEnsureVirtioAddress(&releaseaddr, vm, &dev, "input") < 0) + return -1; + } else if (input->bus == VIR_DOMAIN_INPUT_BUS_USB) { + if (priv->usbaddrs) { + if (virDomainUSBAddressEnsure(priv->usbaddrs, &input->info) < 0) + goto cleanup; + releaseaddr = true; + } + } + + if (qemuAssignDeviceInputAlias(vm->def, input, -1) < 0) + goto cleanup; + + if (qemuBuildInputDevStr(&devstr, vm->def, input, priv->qemuCaps) < 0) + goto cleanup; + + if (VIR_REALLOC_N(vm->def->inputs, vm->def->ninputs + 1) < 0) + goto cleanup; + + qemuDomainObjEnterMonitor(driver, vm); + if (qemuMonitorAddDevice(priv->mon, devstr) < 0) + goto exit_monitor; + + if (qemuDomainObjExitMonitor(driver, vm) < 0) { + releaseaddr = false; + goto cleanup; + } + + VIR_APPEND_ELEMENT_COPY_INPLACE(vm->def->inputs, vm->def->ninputs, input); + + ret = 0; + releaseaddr = false; + + audit: + virDomainAuditInput(vm, input, "attach", ret == 0); + + cleanup: + if (releaseaddr) + qemuDomainReleaseDeviceAddress(vm, &input->info, NULL); + + VIR_FREE(devstr); + return ret; + + exit_monitor: + if (qemuDomainObjExitMonitor(driver, vm) < 0) { + releaseaddr = false; + goto cleanup; + } + goto audit; +} + + static int qemuDomainChangeNetBridge(virDomainObjPtr vm, virDomainNetDefPtr olddev, diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index 3455832d6..985b7495b 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -125,6 +125,11 @@ int qemuDomainDetachShmemDevice(virQEMUDriverPtr driver, int qemuDomainDetachWatchdog(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainWatchdogDefPtr watchdog); + +int qemuDomainAttachInputDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainInputDefPtr input); + int qemuDomainAttachLease(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainLeaseDefPtr lease); -- 2.13.0

On 10/17/2017 11:04 AM, Ján Tomko wrote: Another aha moment ;-)... No issue here... So none of the Remove* API's need to pass @driver... doh! Never even considered that - the first non-lemming ;-)! John [...]