[libvirt] [PATCH 10/10] network: internal API functions to manage assignment of physdev to guest

[Laine Stump]

On 07/08/2011 04:37 PM, Eric Blake wrote:
> On 07/05/2011 01:45 AM, Laine Stump wrote:
>> The network driver needs to assign physical devices for use by modes
>> that use macvtap, keeping track of which physical devices are in use
>> (and how many instances, when the devices can be shared). Three calls
>> are added:
>>
>> networkAllocateActualDevice - finds a physical device for use by the
>> domain, and sets up the virDomainActualNetDef accordingly.
>>
>> networkNotifyActualDevice - assumes that the domain was already
>> running, but libvirtd was restarted, and needs to be notified by each
>> already-running domain about what interfaces they are using.
>>
>> networkReleaseActualDevice - decrements the usage count of the
>> allocated physical device, and frees the virDomainActualNetDef to
>> avoid later accidentally using the device.
>>
>> bridge_driver.[hc] - the new APIs
>>
>> qemu_(command|driver|hotplug|process).c - add calls to the above APIs
>>      in the appropriate places.
>>
>> tests/Makefile.am - need to include libvirt_driver_network.la whenever
>>      libvirt_driver_qemu.la is linked, to avoid unreferenced symbols
>>      (in functions that are never called by the test programs...)
>> ---
>>   src/network/bridge_driver.c |  398 +++++++++++++++++++++++++++++++++++++++++++
>>   src/network/bridge_driver.h |    6 +
>>   src/qemu/qemu_command.c     |   11 ++
>>   src/qemu/qemu_hotplug.c     |   41 +++--
>>   src/qemu/qemu_process.c     |   26 +++-
>>   tests/Makefile.am           |   12 +-
>>   6 files changed, 476 insertions(+), 18 deletions(-)
>> +
>> +        /* Find the most specific virtportprofile and copy it */
>> +        if (iface->data.network.virtPortProfile) {
>> +            virtport = iface->data.network.virtPortProfile;
>> +        } else {
>> +            virPortGroupDefPtr portgroup
>> +               = virPortGroupFindByName(netdef, iface->data.network.portgroup);
>> +            if (portgroup)
>> +                virtport = portgroup->virtPortProfile;
> Nit: Indentation looks interesting there - the line starting with = has
> one less space.  Perhaps something to do with how your preferred editor
> splits assignments.  My editor uses 4 spaces instead of 3 in that instance.


It defaults to a different style, and I sometimes forget to switch.


>> +int
>> +networkNotifyActualDevice(virDomainNetDefPtr iface)
>> +{
>> +
>> +        /* PASSTHROUGH mode, and PRIVATE Mode + 802.1Qbh both require
>> +         * exclusive access to a device, so current usageCount must be
>> +         * 0 in those cases.
>> +         */
>> +        if ((dev->usageCount>  0)&&
>> +            ((netdef->forwardType == VIR_NETWORK_FORWARD_PASSTHROUGH) ||
>> +             ((netdef->forwardType == VIR_NETWORK_FORWARD_PRIVATE)&&
>> +              iface->data.network.actual->data.direct.virtPortProfile&&
>> +              (iface->data.network.actual->data.direct.virtPortProfile->virtPortType
>> +               == VIR_VIRTUALPORT_8021QBH)))) {
>> +            networkReportError(VIR_ERR_INTERNAL_ERROR,
>> +                               _("network '%s' claims dev='%s' is already in use by a different domain"),
> Do we have a data race here?
>
> Suppose that libvirtd is restarted while one VM is already using device
> 0.  Is there any chance that if I'm fast enough, I can cause the
> creation of a second domain, and that second domain will go to pick from
> the interface pool before the notification pass of the libvirtd restart
> has completed, and mistakenly claim device 0?
>
> That is, I think we need to somehow guarantee (if we don't already) that
> no new domains can be created until after all existing domains have been
> reloaded on a libvirtd restart.

I was concerned about this at first too.

Here's the call chain down to this function (it's only called from one 
place):

networkNotifyActualDevice
qemuProcessNotifyNets
qemuProcessReconnect
qemuProcessReconnectAll
qemudStartup <- aka the "initialize" function of the qemu state driver.


At the top of qemudStartup, the lock for the driver is initialized, then 
locked. After this, a ton of stuff is initialized, including calling 
qemuProcessReconnectAll(), then the worker thread pool is created, and 
finally the driver lock is released. So as far as I understand it, it's 
not possible for a new domain to be created until after this is all 
finished.