On Mon, Mar 23, 2015 at 05:46:23PM -0400, John Ferlan wrote:
On 03/17/2015 07:41 AM, Ján Tomko wrote:
Instead of always using controller 0 and incrementing port number, respect the maximum port numbers of controllers and use all of them.
Ports for virtio consoles are quietly reserved, but not formatted (neither in XML nor on QEMU command line).
Also rejects duplicate virtio-serial addresses. https://bugzilla.redhat.com/show_bug.cgi?id=890606 https://bugzilla.redhat.com/show_bug.cgi?id=1076708 --- src/conf/domain_conf.c | 29 ---------- src/qemu/qemu_command.c | 63 ++++++++++++++++++++++ src/qemu/qemu_domain.c | 1 + src/qemu/qemu_domain.h | 1 + src/qemu/qemu_process.c | 2 + .../qemuxml2argv-channel-virtio-auto.args | 8 +-- .../qemuxml2argv-channel-virtio-autoassign.args | 10 ++-- .../qemuxml2xmlout-channel-virtio-auto.xml | 10 ++-- 8 files changed, 81 insertions(+), 43 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 02105c3..e7f86e1 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1399,6 +1399,65 @@ qemuAssignSpaprVIOAddress(virDomainDefPtr def, virDomainDeviceInfoPtr info, return 0; }
+ +static int +qemuDomainAssignVirtioSerialAddresses(virDomainDefPtr def, + virDomainObjPtr obj) +{ + int ret = -1; + size_t i; + virDomainVirtioSerialAddrSetPtr addrs = NULL; + qemuDomainObjPrivatePtr priv = NULL; + + if (!(addrs = virDomainVirtioSerialAddrSetCreate())) + goto cleanup;
Coverity determines addrs != NULL, but
+ + if (virDomainVirtioSerialAddrSetAddControllers(addrs, def) < 0) + goto cleanup; + + if (virDomainDeviceInfoIterate(def, virDomainVirtioSerialAddrReserve, + addrs) < 0) + goto cleanup; + + VIR_DEBUG("Finished reserving existing ports"); + + for (i = 0; i < def->nconsoles; i++) { + virDomainChrDefPtr chr = def->consoles[i]; + if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && + chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO) { + if (virDomainVirtioSerialAddrAssign(addrs, &chr->info, true) < 0) + goto cleanup; + } + } + + for (i = 0; i < def->nchannels; i++) { + virDomainChrDefPtr chr = def->channels[i]; + if (chr->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL && + chr->info.addr.vioserial.port == 0 && + virDomainVirtioSerialAddrAssign(addrs, &chr->info, false) < 0) + goto cleanup; + } + + if (obj && obj->privateData) { + priv = obj->privateData; + if (addrs) {
There's a check here where the "else" is DEADCODE
Right. The address set should only be generated if there is a virtio-serial controller present.
+ /* if this is the live domain object, we persist the addresses */ + virDomainVirtioSerialAddrSetFree(priv->vioserialaddrs); + priv->persistentAddrs = 1; + priv->vioserialaddrs = addrs; + addrs = NULL; + } else { + priv->persistentAddrs = 0; + } + } + ret = 0;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ae315df..5589f22 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5205,6 +5205,8 @@ void qemuProcessStop(virQEMUDriverPtr driver, virDomainDefClearCCWAddresses(vm->def); virDomainCCWAddressSetFree(priv->ccwaddrs); priv->ccwaddrs = NULL; + virDomainVirtioSerialAddrSetFree(priv->vioserialaddrs); + priv->vioserialaddrs = NULL; }
qemuDomainReAttachHostDevices(driver, vm->def);
Mostly for my edification... These examples previously although indicating they were "auto-assign" (of sorts) really weren't - they were more force-assign for each example.
Force-assign after this patch? Otherwise I don't understand.
The way to auto-assign is by setting port='0', correct?
Yes.
However, I'm still missing something from the *auto.args output. It seems the controller='#' is ignored and I guess I don't understand that...
I must've overlooked that. It shouldn't be a problem to take it as a hint in virDomainVirtioSerialAddrAssign.
Sure "port='0'" (meaning first available port on the controller), but I would have expected to see :
kvm.port.0 use "virtio.serial.0.0,nr=1..." (which it does) kvm.port.foo use "virtio.serial.1.0,nr=1..." (on controller 0?, but XML has controller='1') kvm.port.bar use "virtio.serial.1.0,nr=3..." (which it does) kvm.port.wizz use "virtio.serial.0.0,nr=2" (incorrect due to others) kvm.port.ooh use "virtio.serial.1.0,nr=2", (on controller 0?, but XML has controller='1' kvm.port.lla use "virtio.serial.2.0,nr=1" (on controller 0?, but XML has controller='2')
Now if been if "lla" used controller='0', then I assume would nr=4 be chosen since 1,2 were auto-assigned, 3 was specifically assigned, thus 4 would be the next one.
Continuing with that same logic, the *-autoassign example could have shown that if controller='0',port='2' and 'controller='1',port='1' were preassigned, then the controllers/ports assigned would be 0.0,nr=1, 0.0,nr=3, 0.0,nr=4, 1.0,nr=2 (since only 4 ports on controller='0' can be used w/ 2 be static and controller='1' having port '1' already in use).
nr=4 is out of bounds for a controller with 4 ports. The ports are numbered from 0, but port number 0 can only be used for virtconsoles, not channels. Jan