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

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



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.


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