6:41 a.m.
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