[libvirt] [PATCH 0/3] RFC: qemu: drop vioserialaddr cache

[Cole Robinson]

On 05/16/2016 10:05 AM, Laine Stump wrote:
> On 05/14/2016 02:25 PM, Cole Robinson wrote:
>> qemuDomainObjPrivate caches three lists of device addresses:
>>
>>      virDomainPCIAddressSetPtr pciaddrs;
>>      virDomainCCWAddressSetPtr ccwaddrs;
>>      virDomainVirtioSerialAddrSetPtr vioserialaddrs;
>>
>> Yet I can't quite tell what issue they fix... they are only used
>> at hotplug time for checking for address collisions, however it
>> appears that we can generate those lists on demand from the runtime
>> XML, which contains all the info we need.
>>
>> In truth I only looked deeply at the vioserialaddrs list... perhaps
>> PCI has more to it. But at least for virtio serial it looks like
>> this caching can be dropped. CCing jtomko who originally added it
>>
>> If this is acceptable, dropping all the caching will be a step
>> towards unifying all uses of qemuDomainAssignAddresses, rather
>> than sprinkling around a dozen call sites throughout the code.
> 
> FWIW,  when I was doing stuff that touched address assignment, I noticed that
> priv->persistentAddrs was set to 1/0 by each of the
> qemuDomainAssign*Addresses() functions without regard to whether or not it had
> already been set by one of the other qemuDomainAssign*Addresses() functions.
> This meant that, for example, the VirtioSerial version could set
> persistentAddrs = 1, and then the PCI version could reset it back to 0. Since
> nobody had complained and I truthfully don't know what the usefulness of the
> cache is, I didn't touch it. It does look like any domain that has no PCI
> addresses ends up with persistentAddrs == 0, so probably the cache is empty
> anyway (? I guess. I didn't actually look at what's behind that faux-boolean).
> 

Hmm. Yeah that is definitely wrong. And that just makes this thing even more
confusing... persistentAddrs is only used in a qemu_domain.c to trigger 1)
freeing the address caches, and 2) clearing addresses from the XML? no idea
what the latter bit is about. And the logic of using one boolean to cover both
PCI and CCW is definitely wrong here at least

I suspect when the PCI address caching was added a long time back it actually
served a purpose, but I don't think it's relevant anymore, probably due to
unconditionally assigning addresses for every qemu VM. Maybe it was
conditional at some point. CCIng danpb too, maybe he knows

- Cole