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

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.

> +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.