Re: [PATCHv2 3/5] Allocate virtio-serial addresses when starting a domain

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]