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

Reply via email to