[PATCH v6 0/3] Fix virtio console port assignment issue
Changelog: --- v6: - Fixed issue with console auto port index starting from 1 instead of 0 --- v5: - Added xml tests to tests/qemuxmlconfdata - Fixed virito -> virtio typo in commit message --- v4: - Update commit messages --- v3: - Added Reviewed-By - Included CI Results Link --- v2: - Split patch into two commits - Added fixes tag --- This libvirt patch series does the following: 1. fixes an issue with virtio console device port auto assignment on virtio-serial buses 2. updates console port reservation comment and changes the allowZero variable to allowPortZero for clarity 3. Adds tests for virtio console on the vioserial bus Currently in libvirt, a virtio console device cannot be auto assigned a port number greater than zero on a virtio-serial bus. This leads to port collision errors when adding more than 1 virtio console device on a single virtio-serial bus. After applying this patch, one can add multiple console ports under a single virtio-serial bus. Here is a link to CI results for this series: https://gitlab.com/aaronbmalik/libvirt/-/pipelines/1942021076 Aaron M. Brown (3): tests: Add console-virtio-vioserial tests domain_addr: Fix virtio console port autoassign on virtio-serial bus domain_addr: update virtconsole port reservation comment and allowZero var src/conf/domain_addr.c | 50 ++++++++++----- ...rial-autoassign-address.x86_64-latest.args | 44 +++++++++++++ ...erial-autoassign-address.x86_64-latest.xml | 63 +++++++++++++++++++ ...nsole-virtio-serial-autoassign-address.xml | 48 ++++++++++++++ tests/qemuxmlconftest.c | 1 + 5 files changed, 192 insertions(+), 14 deletions(-) create mode 100644 tests/qemuxmlconfdata/console-virtio-serial-autoassign-address.x86_64-latest.args create mode 100644 tests/qemuxmlconfdata/console-virtio-serial-autoassign-address.x86_64-latest.xml create mode 100644 tests/qemuxmlconfdata/console-virtio-serial-autoassign-address.xml -- 2.39.5 (Apple Git-154)
Add test coverage for multiple virtio consoles on a virtio-serial controller. This test makes sure that multiple virtconsoles get auto-assigned appropriate port numbers on a virtio-serial-bus Signed-off-by: Aaron M. Brown <aaronmbr@linux.ibm.com> --- ...rial-autoassign-address.x86_64-latest.args | 44 +++++++++++++ ...erial-autoassign-address.x86_64-latest.xml | 63 +++++++++++++++++++ ...nsole-virtio-serial-autoassign-address.xml | 48 ++++++++++++++ tests/qemuxmlconftest.c | 1 + 4 files changed, 156 insertions(+) create mode 100644 tests/qemuxmlconfdata/console-virtio-serial-autoassign-address.x86_64-latest.args create mode 100644 tests/qemuxmlconfdata/console-virtio-serial-autoassign-address.x86_64-latest.xml create mode 100644 tests/qemuxmlconfdata/console-virtio-serial-autoassign-address.xml diff --git a/tests/qemuxmlconfdata/console-virtio-serial-autoassign-address.x86_64-latest.args b/tests/qemuxmlconfdata/console-virtio-serial-autoassign-address.x86_64-latest.args new file mode 100644 index 0000000000..f987168477 --- /dev/null +++ b/tests/qemuxmlconfdata/console-virtio-serial-autoassign-address.x86_64-latest.args @@ -0,0 +1,44 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1 \ +USER=test \ +LOGNAME=test \ +XDG_DATA_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.local/share \ +XDG_CACHE_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.cache \ +XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \ +/usr/bin/qemu-system-x86_64 \ +-name guest=QEMUGuest1,debug-threads=on \ +-S \ +-object '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/var/lib/libvirt/qemu/domain--1-QEMUGuest1/master-key.aes"}' \ +-machine pc,usb=off,dump-guest-core=off,memory-backend=pc.ram,acpi=off \ +-accel tcg \ +-cpu qemu64 \ +-m size=219136k \ +-object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":224395264}' \ +-overcommit mem-lock=off \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-boot strict=on \ +-device '{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \ +-device '{"driver":"virtio-serial-pci","id":"virtio-serial0","bus":"pci.0","addr":"0x2"}' \ +-blockdev '{"driver":"host_device","filename":"/dev/HostVG/QEMUGuest1","node-name":"libvirt-1-storage","read-only":false}' \ +-device '{"driver":"ide-hd","bus":"ide.0","unit":0,"drive":"libvirt-1-storage","id":"ide0-0-0","bootindex":1}' \ +-chardev pty,id=charserial0 \ +-device '{"driver":"isa-serial","chardev":"charserial0","id":"serial0","index":0}' \ +-chardev pty,id=charconsole1 \ +-device '{"driver":"virtconsole","bus":"virtio-serial0.0","nr":0,"chardev":"charconsole1","id":"console1"}' \ +-chardev pty,id=charconsole2 \ +-device '{"driver":"virtconsole","bus":"virtio-serial0.0","nr":0,"chardev":"charconsole2","id":"console2"}' \ +-chardev pty,id=charconsole3 \ +-device '{"driver":"virtconsole","bus":"virtio-serial0.0","nr":0,"chardev":"charconsole3","id":"console3"}' \ +-audiodev '{"id":"audio1","driver":"none"}' \ +-device '{"driver":"virtio-balloon-pci","id":"balloon0","bus":"pci.0","addr":"0x3"}' \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxmlconfdata/console-virtio-serial-autoassign-address.x86_64-latest.xml b/tests/qemuxmlconfdata/console-virtio-serial-autoassign-address.x86_64-latest.xml new file mode 100644 index 0000000000..acee9999af --- /dev/null +++ b/tests/qemuxmlconfdata/console-virtio-serial-autoassign-address.x86_64-latest.xml @@ -0,0 +1,63 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static' cpuset='1-4,8-20,525'>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <cpu mode='custom' match='exact' check='none'> + <model fallback='forbid'>qemu64</model> + </cpu> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0' model='piix3-uhci'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='virtio-serial' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <serial type='pty'> + <target type='isa-serial' port='0'> + <model name='isa-serial'/> + </target> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + </console> + <console type='pty'> + <target type='virtio' port='1'/> + <address type='virtio-serial' controller='0' bus='0' port='0'/> + </console> + <console type='pty'> + <target type='virtio' port='2'/> + <address type='virtio-serial' controller='0' bus='0' port='0'/> + </console> + <console type='pty'> + <target type='virtio' port='3'/> + <address type='virtio-serial' controller='0' bus='0' port='0'/> + </console> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <audio id='1' type='none'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxmlconfdata/console-virtio-serial-autoassign-address.xml b/tests/qemuxmlconfdata/console-virtio-serial-autoassign-address.xml new file mode 100644 index 0000000000..229572156e --- /dev/null +++ b/tests/qemuxmlconfdata/console-virtio-serial-autoassign-address.xml @@ -0,0 +1,48 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static' cpuset='1-4,8-20,525'>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='virtio-serial' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <serial type='pty'> + <target port='0'/> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + </console> + <console type='pty'> + <target type='virtio' port='1'/> + <address type='virtio-serial' controller='0'/> + </console> + <console type='pty'> + <target type='virtio' port='2'/> + <address type='virtio-serial' controller='0'/> + </console> + <console type='pty'> + <target type='virtio' port='3'/> + <address type='virtio-serial' controller='0'/> + </console> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c index aeca353437..a614ba70fc 100644 --- a/tests/qemuxmlconftest.c +++ b/tests/qemuxmlconftest.c @@ -1906,6 +1906,7 @@ mymain(void) DO_TEST_CAPS_LATEST("channel-virtio-autoadd"); DO_TEST_CAPS_LATEST("console-virtio"); DO_TEST_CAPS_LATEST("console-virtio-many"); + DO_TEST_CAPS_LATEST("console-virtio-serial-autoassign-address"); DO_TEST_CAPS_ARCH_LATEST("console-virtio-ccw", "s390x"); DO_TEST_CAPS_LATEST("console-virtio-unix"); DO_TEST_CAPS_ARCH_LATEST("console-sclp", "s390x"); -- 2.39.5 (Apple Git-154)
The summary line mentions a different name than the one used for the test. On Tue, Jul 22, 2025 at 13:50:18 -0400, Aaron M. Brown wrote:
Add test coverage for multiple virtio consoles on a virtio-serial controller. This test makes sure that multiple virtconsoles get auto-assigned appropriate port numbers on a virtio-serial-bus
Signed-off-by: Aaron M. Brown <aaronmbr@linux.ibm.com> --- ...rial-autoassign-address.x86_64-latest.args | 44 +++++++++++++ ...erial-autoassign-address.x86_64-latest.xml | 63 +++++++++++++++++++ ...nsole-virtio-serial-autoassign-address.xml | 48 ++++++++++++++ tests/qemuxmlconftest.c | 1 + 4 files changed, 156 insertions(+) create mode 100644 tests/qemuxmlconfdata/console-virtio-serial-autoassign-address.x86_64-latest.args create mode 100644 tests/qemuxmlconfdata/console-virtio-serial-autoassign-address.x86_64-latest.xml create mode 100644 tests/qemuxmlconfdata/console-virtio-serial-autoassign-address.xml
[...]
diff --git a/tests/qemuxmlconfdata/console-virtio-serial-autoassign-address.xml b/tests/qemuxmlconfdata/console-virtio-serial-autoassign-address.xml new file mode 100644 index 0000000000..229572156e --- /dev/null +++ b/tests/qemuxmlconfdata/console-virtio-serial-autoassign-address.xml @@ -0,0 +1,48 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static' cpuset='1-4,8-20,525'>1</vcpu>
I'll also drop these definitions ...
+ <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk>
... which are not strictly needed to test consoles.
+ <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='virtio-serial' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <serial type='pty'> + <target port='0'/> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + </console> + <console type='pty'> + <target type='virtio' port='1'/> + <address type='virtio-serial' controller='0'/> + </console> + <console type='pty'> + <target type='virtio' port='2'/> + <address type='virtio-serial' controller='0'/> + </console> + <console type='pty'> + <target type='virtio' port='3'/> + <address type='virtio-serial' controller='0'/> + </console> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'/> + </devices> +</domain>
Reviewed-by: Peter Krempa <pkrempa@redhat.com> I'll address the few things I've pointed out myself.
This change fixes an issue with virtio console port assignment on virtio-serial buses. Currently, when trying to autoassign a virtio console device, the device cannot be assigned to a port greater than 0 on virtio-serial buses. You will receive the following error: `virtio-serial-bus: A port already exists at id 0` Therefore, the data needs to be passed back into info when allowZero is true. We should also preserve the controller data when allowZero is true, and propagate allowZero into virDomainVirtioSerialAddrNextFromController to get an appropriate startPort. Fixes: 16db8d2e ("Add functions to track virtio-serial addresses") Signed-off-by: Aaron M. Brown <aaronmbr@linux.ibm.com> --- src/conf/domain_addr.c | 30 ++++++++++++++++--- ...rial-autoassign-address.x86_64-latest.args | 4 +-- ...erial-autoassign-address.x86_64-latest.xml | 4 +-- 3 files changed, 30 insertions(+), 8 deletions(-) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 8dfa8feca0..5448d3d078 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -1692,12 +1692,16 @@ virDomainVirtioSerialAddrNext(virDomainDef *def, static int virDomainVirtioSerialAddrNextFromController(virDomainVirtioSerialAddrSet *addrs, - virDomainDeviceVirtioSerialAddress *addr) + virDomainDeviceVirtioSerialAddress *addr, + bool allowZero) { - ssize_t port; + ssize_t port, startPort = 0; ssize_t i; virBitmap *map; + if (allowZero) + startPort = -1; + i = virDomainVirtioSerialAddrFindController(addrs, addr->controller); if (i < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -1707,7 +1711,7 @@ virDomainVirtioSerialAddrNextFromController(virDomainVirtioSerialAddrSet *addrs, } map = addrs->controllers[i]->ports; - if ((port = virBitmapNextClearBit(map, 0)) <= 0) { + if ((port = virBitmapNextClearBit(map, startPort)) < 0) { virReportError(VIR_ERR_XML_ERROR, _("Unable to find a free port on virtio-serial controller %1$u"), addr->controller); @@ -1718,6 +1722,15 @@ virDomainVirtioSerialAddrNextFromController(virDomainVirtioSerialAddrSet *addrs, addr->port = port; VIR_DEBUG("Found free virtio serial controller %u port %u", addr->controller, addr->port); + + /* if this is the first virtconsole, reserve port 0 */ + if (allowZero && port == 0) { + ignore_value(virBitmapSetBit(map, 0)); + VIR_DEBUG( + "Port 0 reserved for the first virtconsole on vioserial controller %1$u", + addr->controller); + } + return 0; } @@ -1732,11 +1745,20 @@ virDomainVirtioSerialAddrAssign(virDomainDef *def, virDomainDeviceInfo *ptr = allowZero ? &nfo : info; ptr->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL; + ptr->addr.vioserial.controller = info->addr.vioserial.controller; if (portOnly) { if (virDomainVirtioSerialAddrNextFromController(addrs, - &ptr->addr.vioserial) < 0) + &ptr->addr.vioserial, + allowZero) < 0) return -1; + + if (ptr == &nfo) { + /* pass the vioserial data back into info as info is used + * later for port assignment */ + info->addr.vioserial = ptr->addr.vioserial; + } + } else { if (virDomainVirtioSerialAddrNext(def, addrs, &ptr->addr.vioserial, allowZero) < 0) diff --git a/tests/qemuxmlconfdata/console-virtio-serial-autoassign-address.x86_64-latest.args b/tests/qemuxmlconfdata/console-virtio-serial-autoassign-address.x86_64-latest.args index f987168477..4f4b95e969 100644 --- a/tests/qemuxmlconfdata/console-virtio-serial-autoassign-address.x86_64-latest.args +++ b/tests/qemuxmlconfdata/console-virtio-serial-autoassign-address.x86_64-latest.args @@ -35,9 +35,9 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \ -chardev pty,id=charconsole1 \ -device '{"driver":"virtconsole","bus":"virtio-serial0.0","nr":0,"chardev":"charconsole1","id":"console1"}' \ -chardev pty,id=charconsole2 \ --device '{"driver":"virtconsole","bus":"virtio-serial0.0","nr":0,"chardev":"charconsole2","id":"console2"}' \ +-device '{"driver":"virtconsole","bus":"virtio-serial0.0","nr":1,"chardev":"charconsole2","id":"console2"}' \ -chardev pty,id=charconsole3 \ --device '{"driver":"virtconsole","bus":"virtio-serial0.0","nr":0,"chardev":"charconsole3","id":"console3"}' \ +-device '{"driver":"virtconsole","bus":"virtio-serial0.0","nr":2,"chardev":"charconsole3","id":"console3"}' \ -audiodev '{"id":"audio1","driver":"none"}' \ -device '{"driver":"virtio-balloon-pci","id":"balloon0","bus":"pci.0","addr":"0x3"}' \ -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ diff --git a/tests/qemuxmlconfdata/console-virtio-serial-autoassign-address.x86_64-latest.xml b/tests/qemuxmlconfdata/console-virtio-serial-autoassign-address.x86_64-latest.xml index acee9999af..712d6c8a56 100644 --- a/tests/qemuxmlconfdata/console-virtio-serial-autoassign-address.x86_64-latest.xml +++ b/tests/qemuxmlconfdata/console-virtio-serial-autoassign-address.x86_64-latest.xml @@ -47,11 +47,11 @@ </console> <console type='pty'> <target type='virtio' port='2'/> - <address type='virtio-serial' controller='0' bus='0' port='0'/> + <address type='virtio-serial' controller='0' bus='0' port='1'/> </console> <console type='pty'> <target type='virtio' port='3'/> - <address type='virtio-serial' controller='0' bus='0' port='0'/> + <address type='virtio-serial' controller='0' bus='0' port='2'/> </console> <input type='mouse' bus='ps2'/> <input type='keyboard' bus='ps2'/> -- 2.39.5 (Apple Git-154)
On Tue, Jul 22, 2025 at 13:50:19 -0400, Aaron M. Brown wrote:
This change fixes an issue with virtio console port assignment on virtio-serial buses. Currently, when trying to autoassign a virtio console device, the device cannot be assigned to a port greater than 0 on virtio-serial buses. You will receive the following error:
`virtio-serial-bus: A port already exists at id 0`
Therefore, the data needs to be passed back into info when allowZero is true. We should also preserve the controller data when allowZero is true, and propagate allowZero into virDomainVirtioSerialAddrNextFromController to get an appropriate startPort.
Fixes: 16db8d2e ("Add functions to track virtio-serial addresses") Signed-off-by: Aaron M. Brown <aaronmbr@linux.ibm.com> --- src/conf/domain_addr.c | 30 ++++++++++++++++--- ...rial-autoassign-address.x86_64-latest.args | 4 +-- ...erial-autoassign-address.x86_64-latest.xml | 4 +-- 3 files changed, 30 insertions(+), 8 deletions(-)
diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 8dfa8feca0..5448d3d078 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -1692,12 +1692,16 @@ virDomainVirtioSerialAddrNext(virDomainDef *def,
static int virDomainVirtioSerialAddrNextFromController(virDomainVirtioSerialAddrSet *addrs, - virDomainDeviceVirtioSerialAddress *addr) + virDomainDeviceVirtioSerialAddress *addr, + bool allowZero) { - ssize_t port; + ssize_t port, startPort = 0;
One definition per line.
ssize_t i; virBitmap *map;
+ if (allowZero) + startPort = -1; + i = virDomainVirtioSerialAddrFindController(addrs, addr->controller); if (i < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -1707,7 +1711,7 @@ virDomainVirtioSerialAddrNextFromController(virDomainVirtioSerialAddrSet *addrs, }
map = addrs->controllers[i]->ports; - if ((port = virBitmapNextClearBit(map, 0)) <= 0) { + if ((port = virBitmapNextClearBit(map, startPort)) < 0) { virReportError(VIR_ERR_XML_ERROR, _("Unable to find a free port on virtio-serial controller %1$u"), addr->controller); @@ -1718,6 +1722,15 @@ virDomainVirtioSerialAddrNextFromController(virDomainVirtioSerialAddrSet *addrs, addr->port = port; VIR_DEBUG("Found free virtio serial controller %u port %u", addr->controller, addr->port); + + /* if this is the first virtconsole, reserve port 0 */ + if (allowZero && port == 0) { + ignore_value(virBitmapSetBit(map, 0)); + VIR_DEBUG( + "Port 0 reserved for the first virtconsole on vioserial controller %1$u", + addr->controller); + }
This is suspicious because this function until now just returned the next eligible port, but now it's also reserving it which doesn't seem to fit well here. Can you please explain this? Also the spacing is weird, but I can fix that.
On Tue, Jul 22, 2025 at 13:50:19 -0400, Aaron M. Brown wrote:
One definition per line.
Okay I will remember that going forward!
This is suspicious because this function until now just returned the next eligible port, but now it's also reserving it which doesn't seem to fit well here.
Can you please explain this?
Yes, I agree with you Peter, it is a bit unfitting, we can't process a port assigned to port 0 in virDomainVirtioSerialAddrReserve as it fails the virDomainVirtioSerialAddrIsComplete check, because that check makes sure that `info->addr.vioserial.port != 0` so such a port would show as imcomplete. And if we skip over reserving this port, that means the bitmap would show a clear bit at the first position for every subsequent device on the controller
On Thu, Jul 24, 2025 at 12:48:06 -0000, Aaron Brown wrote:
On Tue, Jul 22, 2025 at 13:50:19 -0400, Aaron M. Brown wrote:
One definition per line.
Okay I will remember that going forward!
This is suspicious because this function until now just returned the next eligible port, but now it's also reserving it which doesn't seem to fit well here.
Can you please explain this?
Yes, I agree with you Peter, it is a bit unfitting, we can't process a port assigned to port 0 in virDomainVirtioSerialAddrReserve as it fails the virDomainVirtioSerialAddrIsComplete check, because that check makes sure that `info->addr.vioserial.port != 0` so such a port would show as imcomplete.
And if we skip over reserving this port, that means the bitmap would show a clear bit at the first position for every subsequent device on the controller
So while I agree that it needs to work properly with the reservation code, with the way the code is structured I don't think we should hide something which reserves ports into code that is not expected to do that.
So while I agree that it needs to work properly with the reservation code, with the way the code is structured I don't think we should hide something which reserves ports into code that is not expected to do that.
Yeah, I see what you're saying. Given this, there are some options available to us. 1. We could move that conditional out into virDomainVirtioSerialAddrAssign, that might require a similar query for the controller, to ultimately get the map. 2. We could possibly modify virDomainVirtioSerialAddrReserve, and/or virDomainVirtioSerialAddrIsComplete Im open to any other suggestions as well ! Regards, Aaron
Rename variable "allowZero" to "allowPortZero" for clarity and update the virtconsole port reservation comment, as port 0 is reserved for the first virtconsole unless specified. Signed-off-by: Aaron M. Brown <aaronmbr@linux.ibm.com> --- src/conf/domain_addr.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 5448d3d078..1bf0530688 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -1638,14 +1638,14 @@ static int virDomainVirtioSerialAddrNext(virDomainDef *def, virDomainVirtioSerialAddrSet *addrs, virDomainDeviceVirtioSerialAddress *addr, - bool allowZero) + bool allowPortZero) { ssize_t port, startPort = 0; ssize_t i; unsigned int controller; - /* port number 0 is reserved for virtconsoles */ - if (allowZero) + /* port number 0 is reserved for the first virtconsole */ + if (allowPortZero) startPort = -1; if (addrs->ncontrollers == 0) { @@ -1693,13 +1693,13 @@ virDomainVirtioSerialAddrNext(virDomainDef *def, static int virDomainVirtioSerialAddrNextFromController(virDomainVirtioSerialAddrSet *addrs, virDomainDeviceVirtioSerialAddress *addr, - bool allowZero) + bool allowPortZero) { ssize_t port, startPort = 0; ssize_t i; virBitmap *map; - if (allowZero) + if (allowPortZero) startPort = -1; i = virDomainVirtioSerialAddrFindController(addrs, addr->controller); @@ -1724,7 +1724,7 @@ virDomainVirtioSerialAddrNextFromController(virDomainVirtioSerialAddrSet *addrs, addr->port); /* if this is the first virtconsole, reserve port 0 */ - if (allowZero && port == 0) { + if (allowPortZero && port == 0) { ignore_value(virBitmapSetBit(map, 0)); VIR_DEBUG( "Port 0 reserved for the first virtconsole on vioserial controller %1$u", @@ -1738,11 +1738,11 @@ static int ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) virDomainVirtioSerialAddrAssign(virDomainDef *def, virDomainVirtioSerialAddrSet *addrs, virDomainDeviceInfo *info, - bool allowZero, + bool allowPortZero, bool portOnly) { virDomainDeviceInfo nfo = { 0 }; - virDomainDeviceInfo *ptr = allowZero ? &nfo : info; + virDomainDeviceInfo *ptr = allowPortZero ? &nfo : info; ptr->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL; ptr->addr.vioserial.controller = info->addr.vioserial.controller; @@ -1750,7 +1750,7 @@ virDomainVirtioSerialAddrAssign(virDomainDef *def, if (portOnly) { if (virDomainVirtioSerialAddrNextFromController(addrs, &ptr->addr.vioserial, - allowZero) < 0) + allowPortZero) < 0) return -1; if (ptr == &nfo) { @@ -1761,7 +1761,7 @@ virDomainVirtioSerialAddrAssign(virDomainDef *def, } else { if (virDomainVirtioSerialAddrNext(def, addrs, &ptr->addr.vioserial, - allowZero) < 0) + allowPortZero) < 0) return -1; } @@ -1780,20 +1780,20 @@ int virDomainVirtioSerialAddrAutoAssignFromCache(virDomainDef *def, virDomainVirtioSerialAddrSet *addrs, virDomainDeviceInfo *info, - bool allowZero) + bool allowPortZero) { bool portOnly = info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL; if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL && info->addr.vioserial.port) return virDomainVirtioSerialAddrReserve(NULL, NULL, info, addrs); else - return virDomainVirtioSerialAddrAssign(def, addrs, info, allowZero, portOnly); + return virDomainVirtioSerialAddrAssign(def, addrs, info, allowPortZero, portOnly); } int virDomainVirtioSerialAddrAutoAssign(virDomainDef *def, virDomainDeviceInfo *info, - bool allowZero) + bool allowPortZero) { virDomainVirtioSerialAddrSet *addrs = NULL; int ret = -1; @@ -1801,7 +1801,7 @@ virDomainVirtioSerialAddrAutoAssign(virDomainDef *def, if (!(addrs = virDomainVirtioSerialAddrSetCreateFromDomain(def))) goto cleanup; - if (virDomainVirtioSerialAddrAutoAssignFromCache(def, addrs, info, allowZero) < 0) + if (virDomainVirtioSerialAddrAutoAssignFromCache(def, addrs, info, allowPortZero) < 0) goto cleanup; ret = 0; -- 2.39.5 (Apple Git-154)
participants (3)
-
Aaron Brown -
Aaron M. Brown
-
Peter Krempa