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/conf/domain_conf.c b/src/conf/domain_conf.c
> index c75b543..89357d2 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -3402,21 +3402,6 @@
> virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev,
>
> chr->target.port = maxport + 1;
> }
> -
> - if (chr->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL &&
> - chr->info.addr.vioserial.port == 0) {
> - int maxport = 0;
> -
> - for (i = 0; i < cnt; i++) {
> - const virDomainChrDef *thischr = arrPtr[i];
> - if (thischr->info.type ==
> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL &&
> - thischr->info.addr.vioserial.controller ==
> chr->info.addr.vioserial.controller &&
> - thischr->info.addr.vioserial.bus ==
> chr->info.addr.vioserial.bus &&
> - (int)thischr->info.addr.vioserial.port > maxport)
> - maxport = thischr->info.addr.vioserial.port;
> - }
> - chr->info.addr.vioserial.port = maxport + 1;
> - }
> }
>
> /* set default path for virtio-rng "random" backend to /dev/random */
> @@ -14526,20 +14511,6 @@ virDomainDefParseXML(xmlDocPtr xml,
> chr->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO &&
> chr->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
> chr->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL;
> -
> - if (chr->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL &&
> - chr->info.addr.vioserial.port == 0) {
> - int maxport = 0;
> - for (j = 0; j < i; j++) {
> - virDomainChrDefPtr thischr = def->channels[j];
> - if (thischr->info.type ==
> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL &&
> - thischr->info.addr.vioserial.controller ==
> chr->info.addr.vioserial.controller &&
> - thischr->info.addr.vioserial.bus ==
> chr->info.addr.vioserial.bus &&
> - (int)thischr->info.addr.vioserial.port > maxport)
> - maxport = thischr->info.addr.vioserial.port;
> - }
> - chr->info.addr.vioserial.port = maxport + 1;
> - }
> }
> VIR_FREE(nodes);
>
> 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
> + /* 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;
> +
> + cleanup:
> + virDomainVirtioSerialAddrSetFree(addrs);
> + return ret;
> +}
> +
> +
> int qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def,
> virQEMUCapsPtr qemuCaps)
> {
> @@ -1645,6 +1704,10 @@ int qemuDomainAssignAddresses(virDomainDefPtr def,
> {
> int rc;
>
> + rc = qemuDomainAssignVirtioSerialAddresses(def, obj);
> + if (rc)
> + return rc;
> +
> rc = qemuDomainAssignSpaprVIOAddresses(def, qemuCaps);
> if (rc)
> return rc;
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 2eacef2..cb2c166 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -431,6 +431,7 @@ qemuDomainObjPrivateFree(void *data)
> virCgroupFree(&priv->cgroup);
> virDomainPCIAddressSetFree(priv->pciaddrs);
> virDomainCCWAddressSetFree(priv->ccwaddrs);
> + virDomainVirtioSerialAddrSetFree(priv->vioserialaddrs);
> virDomainChrSourceDefFree(priv->monConfig);
> qemuDomainObjFreeJob(priv);
> VIR_FREE(priv->vcpupids);
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index ba8d398..9ad88a8 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -159,6 +159,7 @@ struct _qemuDomainObjPrivate {
>
> virDomainPCIAddressSetPtr pciaddrs;
> virDomainCCWAddressSetPtr ccwaddrs;
> + virDomainVirtioSerialAddrSetPtr vioserialaddrs;
> int persistentAddrs;
>
> virQEMUCapsPtr qemuCaps;
> 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. The way to auto-assign is by
setting port='0', correct?
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... 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).
John
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-auto.args
> b/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-auto.args
> index f7d7409..71edfae 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-auto.args
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-auto.args
> @@ -9,14 +9,14 @@ virtio-serial-pci,id=virtio-serial2,bus=pci.0,addr=0x4 -usb
> -hda \
> /dev/HostVG/QEMUGuest1 -chardev pty,id=charchannel0 -device virtserialport,\
> bus=virtio-serial0.0,nr=1,chardev=charchannel0,id=channel0,\
> name=org.linux-kvm.port.0 -chardev pty,id=charchannel1 -device
> virtserialport,\
> -bus=virtio-serial1.0,nr=1,chardev=charchannel1,id=channel1,\
> +bus=virtio-serial0.0,nr=2,chardev=charchannel1,id=channel1,\
> name=org.linux-kvm.port.foo -chardev pty,id=charchannel2 -device \
> virtserialport,bus=virtio-serial1.0,nr=3,chardev=charchannel2,id=channel2,\
> name=org.linux-kvm.port.bar -chardev pty,id=charchannel3 -device \
> -virtserialport,bus=virtio-serial0.0,nr=2,chardev=charchannel3,id=channel3,\
> +virtserialport,bus=virtio-serial0.0,nr=3,chardev=charchannel3,id=channel3,\
> name=org.linux-kvm.port.wizz -chardev pty,id=charchannel4 -device \
> -virtserialport,bus=virtio-serial1.0,nr=4,chardev=charchannel4,id=channel4,\
> +virtserialport,bus=virtio-serial0.0,nr=4,chardev=charchannel4,id=channel4,\
> name=org.linux-kvm.port.ooh -chardev pty,id=charchannel5 -device \
> -virtserialport,bus=virtio-serial2.0,nr=1,chardev=charchannel5,id=channel5,\
> +virtserialport,bus=virtio-serial0.0,nr=5,chardev=charchannel5,id=channel5,\
> name=org.linux-kvm.port.lla -device virtio-balloon-pci,id=balloon0,\
> bus=pci.0,addr=0x5
> diff --git
> a/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-autoassign.args
> b/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-autoassign.args
> index d64a228..f11039d 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-autoassign.args
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-autoassign.args
> @@ -5,16 +5,16 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test
> QEMU_AUDIO_DRV=none \
> -device virtio-serial-pci,id=virtio-serial0,max_ports=4,vectors=4,bus=pci.0\
> ,addr=0x3 -device virtio-serial-pci,id=virtio-serial1,bus=pci.0,addr=0xa \
> -usb -hda /dev/HostVG/QEMUGuest1 \
> --chardev pty,id=charchannel0 -device
> virtserialport,bus=virtio-serial0.0,nr=1,\
> +-chardev pty,id=charchannel0 -device
> virtserialport,bus=virtio-serial0.0,nr=2,\
> chardev=charchannel0,id=channel0,name=org.linux-kvm.port.0 \
> --chardev pty,id=charchannel1 -device
> virtserialport,bus=virtio-serial0.0,nr=2,\
> +-chardev pty,id=charchannel1 -device
> virtserialport,bus=virtio-serial0.0,nr=3,\
> chardev=charchannel1,id=channel1,name=org.linux-kvm.port.foo \
> -chardev pty,id=charchannel2 -device
> virtserialport,bus=virtio-serial0.0,nr=1,\
> chardev=charchannel2,id=channel2,name=org.linux-kvm.port.bar \
> --chardev pty,id=charchannel3 -device
> virtserialport,bus=virtio-serial0.2,nr=1,\
> +-chardev pty,id=charchannel3 -device
> virtserialport,bus=virtio-serial1.0,nr=1,\
> chardev=charchannel3,id=channel3,name=org.linux-kvm.port.wizz \
> --chardev pty,id=charchannel4 -device
> virtserialport,bus=virtio-serial0.0,nr=3,\
> +-chardev pty,id=charchannel4 -device
> virtserialport,bus=virtio-serial1.0,nr=2,\
> chardev=charchannel4,id=channel4,name=org.linux-kvm.port.ooh \
> --chardev pty,id=charchannel5 -device
> virtserialport,bus=virtio-serial0.0,nr=4,\
> +-chardev pty,id=charchannel5 -device
> virtserialport,bus=virtio-serial1.0,nr=3,\
> chardev=charchannel5,id=channel5,name=org.linux-kvm.port.lla \
> -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4
> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-virtio-auto.xml
> b/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-virtio-auto.xml
> index fd6b852..afe41cf 100644
> --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-virtio-auto.xml
> +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-virtio-auto.xml
> @@ -29,11 +29,11 @@
> <controller type='virtio-serial' index='2'/>
> <channel type='pty'>
> <target type='virtio' name='org.linux-kvm.port.0'/>
> - <address type='virtio-serial' controller='0' bus='0' port='1'/>
> + <address type='virtio-serial' controller='0' bus='0' port='0'/>
> </channel>
> <channel type='pty'>
> <target type='virtio' name='org.linux-kvm.port.foo'/>
> - <address type='virtio-serial' controller='1' bus='0' port='1'/>
> + <address type='virtio-serial' controller='1' bus='0' port='0'/>
> </channel>
> <channel type='pty'>
> <target type='virtio' name='org.linux-kvm.port.bar'/>
> @@ -41,15 +41,15 @@
> </channel>
> <channel type='pty'>
> <target type='virtio' name='org.linux-kvm.port.wizz'/>
> - <address type='virtio-serial' controller='0' bus='0' port='2'/>
> + <address type='virtio-serial' controller='0' bus='0' port='0'/>
> </channel>
> <channel type='pty'>
> <target type='virtio' name='org.linux-kvm.port.ooh'/>
> - <address type='virtio-serial' controller='1' bus='0' port='4'/>
> + <address type='virtio-serial' controller='1' bus='0' port='0'/>
> </channel>
> <channel type='pty'>
> <target type='virtio' name='org.linux-kvm.port.lla'/>
> - <address type='virtio-serial' controller='2' bus='0' port='1'/>
> + <address type='virtio-serial' controller='2' bus='0' port='0'/>
> </channel>
> <memballoon model='virtio'/>
> </devices>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
