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

2011-07-19 Thread 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.


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

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

2011-07-10 Thread Laine Stump 

On 07/08/2011 04:23 PM, Eric Blake wrote:

On 07/05/2011 10:18 PM, Laine Stump wrote:

I realized that PATCH 10/10 would cause the build to fail if someone
did a build --with-qemu but --without-network. I'm squashing the
following into the original patch to remedy that.

I'm not really a fan of putting #if all over the place, but this is
similar to what's done with WITH_MACVTAP, so at least there's
precedence. (This is necessary because this new backend API to the
network driver isn't called via a pointer table filled in at runtime,
as is done with the public API).

Would it be any easier to instead guarantee that even when #if
WITH_NETWORK is false, those same symbols are available as no-ops to
allow compilation to proceed?



That would mean either defining the functions inside qemu, or creating a 
dummy network module replacement to be built when WITH_NETWORK is false. 
I like both of those options even less.




+#if WITH_NETWORK
  /* If appropriate, grab a physical device from the configured
   * network's pool of devices, or resolve bridge device name
   * to the one defined in the network definition.
   */
  if (networkAllocateActualDevice(net)  0)
 goto error;
-
+#endif

That is, make networkAllocateActualDevice() be a no-op that returns 0 if
there is no network support compiled in, and therefore nothing to allocate.



But where would that NOP function live? The natural place for a function 
called networkSomething is in a network driver, but there isn't one.




+++ b/tests/Makefile.am
@@ -319,8 +319,11 @@ endif

  if WITH_QEMU

-qemu_LDADDS = ../src/libvirt_driver_qemu.la \
-   ../src/libvirt_driver_network.la
+qemu_LDADDS = ../src/libvirt_driver_qemu.la
+
+if WITH_NETWORK
+qemu_LDADDS += ../src/libvirt_driver_network.la
+endif

Then again, if you are compiling --without-network, you don't want to
link against a library that won't be built.  That would imply that any
no-op stubs would have to be provided by static inline functions in the
header in the no-network case.


Ah, you're doing this mail On the Road style (stream of consciousness 
with no going back to edit), so I'll respond that way too :-)


Actually, I like this idea - in bridge_driver.h, I can put the function 
declrations inside #if WITH_NETWORK, and have a #else clause that 
contains inlines that return success but do nothing (exactly what is 
needed). That way the code in qemu won't need #if.


I'll do it that way in the next version.

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

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

2011-07-08 Thread Eric Blake 
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.

 +if (!netdef-forwardDev) {
 +networkReportError(VIR_ERR_INTERNAL_ERROR,
 +   _(network '%s' uses a direct mode, but 
 has no forward dev and no interface pool),
 +   netdef-name);
 +goto cleanup;
 +}
 +iface-data.network.actual-data.direct.linkdev
 += strdup(netdef-forwardDev);

And here you used a multiple of four spaces.

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

-- 
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2011-07-05 Thread Laine Stump 
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(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 69d4c35..02d7b8b 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -2702,3 +2702,401 @@ int networkRegister(void) {
 virRegisterStateDriver(networkStateDriver);
 return 0;
 }
+
+//
+
+/* Private API to deal with logical switch capabilities.
+ * These functions are exported so that other parts of libvirt can
+ * call them, but are not part of the public API and not in the
+ * driver's function table. If we ever have more than one network
+ * driver, we will need to present these functions via a second
+ * backend function table.
+ */
+
+/* networkAllocateActualDevice:
+ * @iface: the original NetDef from the domain
+ *
+ * Looks up the network reference by iface, allocates a physical
+ * device from that network (if appropriate), and returns with the
+ * virDomainActualNetDef filled in accordingly. If there are no
+ * changes to be made in the netdef, then just leave the actualdef
+ * empty.
+ *
+ * Returns 0 on success, -1 on failure.
+ */
+int
+networkAllocateActualDevice(virDomainNetDefPtr iface)
+{
+struct network_driver *driver = driverState;
+virNetworkObjPtr network;
+virNetworkDefPtr netdef;
+int ret = -1;
+
+if (iface-type != VIR_DOMAIN_NET_TYPE_NETWORK)
+return 0;
+
+virDomainActualNetDefFree(iface-data.network.actual);
+iface-data.network.actual = NULL;
+
+networkDriverLock(driver);
+network = virNetworkFindByName(driver-networks, 
iface-data.network.name);
+networkDriverUnlock(driver);
+if (!network) {
+networkReportError(VIR_ERR_NO_NETWORK,
+   _(no network with matching name '%s'),
+   iface-data.network.name);
+goto cleanup;
+}
+
+netdef = network-def;
+if ((netdef-forwardType == VIR_NETWORK_FORWARD_BRIDGE) 
+netdef-bridge) {
+
+/* forward type='bridge'/ bridge name='xxx'/
+ * is VIR_DOMAIN_NET_TYPE_BRIDGE
+ */
+
+if (VIR_ALLOC(iface-data.network.actual)  0) {
+virReportOOMError();
+goto cleanup;
+}
+
+iface-data.network.actual-type = VIR_DOMAIN_NET_TYPE_BRIDGE;
+iface-data.network.actual-data.bridge.brname = 
strdup(netdef-bridge);
+if (!iface-data.network.actual-data.bridge.brname) {
+virReportOOMError();
+goto cleanup;
+}
+
+} else if ((netdef-forwardType == VIR_NETWORK_FORWARD_BRIDGE) ||
+   (netdef-forwardType == VIR_NETWORK_FORWARD_PRIVATE) ||
+   (netdef-forwardType == VIR_NETWORK_FORWARD_VEPA) ||
+   (netdef-forwardType == VIR_NETWORK_FORWARD_PASSTHROUGH)) {
+virVirtualPortProfileParamsPtr virtport = NULL;
+
+/* forward type='bridge|private|vepa|passthrough' are all
+ * VIR_DOMAIN_NET_TYPE_DIRECT.
+ */
+
+if (VIR_ALLOC(iface-data.network.actual)  0) {
+virReportOOMError();
+goto cleanup;
+}
+
+/* Set type=direct and appropriate source mode='xxx'/ */
+iface-data.network.actual-type = VIR_DOMAIN_NET_TYPE_DIRECT;
+switch (netdef-forwardType) {
+case VIR_NETWORK_FORWARD_BRIDGE:
+iface-data.network.actual-data.direct.mode = 
VIR_MACVTAP_MODE_BRIDGE;
+break;
+case VIR_NETWORK_FORWARD_PRIVATE:
+iface-data.network.actual-data.direct.mode = 
VIR_MACVTAP_MODE_PRIVATE;
+break;
+case

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

2011-07-05 Thread Laine Stump 
I realized that PATCH 10/10 would cause the build to fail if someone
did a build --with-qemu but --without-network. I'm squashing the
following into the original patch to remedy that.

I'm not really a fan of putting #if all over the place, but this is
similar to what's done with WITH_MACVTAP, so at least there's
precedence. (This is necessary because this new backend API to the
network driver isn't called via a pointer table filled in at runtime,
as is done with the public API).

---
 src/qemu/qemu_command.c |5 -
 src/qemu/qemu_hotplug.c |8 ++--
 src/qemu/qemu_process.c |8 ++--
 tests/Makefile.am   |7 +--
 4 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index d6a0c6d..0b957cb 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3678,13 +3678,14 @@ qemuBuildCommandLine(virConnectPtr conn,
 else
 vlan = i;
 
+#if WITH_NETWORK
 /* If appropriate, grab a physical device from the configured
  * network's pool of devices, or resolve bridge device name
  * to the one defined in the network definition.
  */
 if (networkAllocateActualDevice(net)  0)
goto error;
-
+#endif
 actualType = virDomainNetGetActualType(net);
 if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
 actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) {
@@ -4682,8 +4683,10 @@ qemuBuildCommandLine(virConnectPtr conn,
 virReportOOMError();
  error:
 /* free up any resources in the network driver */
+#if WITH_NETWORK
 for (i = 0 ; i  def-nnets ; i++)
 networkReleaseActualDevice(def-nets[i]);
+#endif
 for (i = 0; i = last_good_net; i++)
 virDomainConfNWFilterTeardown(def-nets[i]);
 virBufferFreeAndReset(rbd_hosts);
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 37cfbef..5f1a424 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -613,13 +613,14 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
 return -1;
 }
 
+#if WITH_NETWORK
 /* If appropriate, grab a physical device from the configured
  * network's pool of devices, or resolve bridge device name
  * to the one defined in the network definition.
  */
 if (networkAllocateActualDevice(net)  0)
 goto cleanup;
-
+#endif
 actualType = virDomainNetGetActualType(net);
 if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
 actualType == VIR_DOMAIN_NET_TYPE_NETWORK) {
@@ -761,7 +762,9 @@ cleanup:
 if (iface_connected)
 virDomainConfNWFilterTeardown(net);
 
+#if WITH_NETWORK
 networkReleaseActualDevice(net);
+#endif
 }
 
 VIR_FREE(nicstr);
@@ -1644,8 +1647,9 @@ int qemuDomainDetachNetDevice(struct qemud_driver *driver,
 }
 }
 
+#if WITH_NETWORK
 networkReleaseActualDevice(detach);
-
+#endif
 if (vm-def-nnets  1) {
 memmove(vm-def-nets + i,
 vm-def-nets + i + 1,
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index f4a57ff..709f187 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2168,6 +2168,7 @@ int qemuProcessStopCPUs(struct qemud_driver *driver, 
virDomainObjPtr vm,
 
 
 
+#if WITH_NETWORK
 static int
 qemuProcessNotifyNets(virDomainDefPtr def)
 {
@@ -2180,7 +2181,7 @@ qemuProcessNotifyNets(virDomainDefPtr def)
 }
 return 0;
 }
-
+#endif
 
 static int
 qemuProcessFiltersInstantiate(virConnectPtr conn,
@@ -2294,9 +2295,10 @@ qemuProcessReconnect(void *payload, const void *name 
ATTRIBUTE_UNUSED, void *opa
 if (virSecurityManagerReserveLabel(driver-securityManager, obj)  0)
 goto error;
 
+#if WITH_NETWORK
 if (qemuProcessNotifyNets(obj-def)  0)
 goto error;
-
+#endif
 if (qemuProcessFiltersInstantiate(conn, obj-def))
 goto error;
 
@@ -2932,7 +2934,9 @@ void qemuProcessStop(struct qemud_driver *driver,
 /* release the physical device (or any other resources used by
  * this interface in the network driver
  */
+#if WITH_NETWORK
 networkReleaseActualDevice(net);
+#endif
 }
 
 retry:
diff --git a/tests/Makefile.am b/tests/Makefile.am
index a9ad759..6afec28 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -319,8 +319,11 @@ endif
 
 if WITH_QEMU
 
-qemu_LDADDS = ../src/libvirt_driver_qemu.la \
-   ../src/libvirt_driver_network.la
+qemu_LDADDS = ../src/libvirt_driver_qemu.la
+
+if WITH_NETWORK
+qemu_LDADDS += ../src/libvirt_driver_network.la
+endif
 
 qemuxml2argvtest_SOURCES = \
qemuxml2argvtest.c testutilsqemu.c testutilsqemu.h \
-- 
1.7.3.4

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