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

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

On 07/20/2011 12:21 AM, Laine Stump wrote:

[sorry for running out of time yesterday, and leaving this series hanging on one patch]

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. When WITH_NETWORK is false, these
functions are all defined inline static in the .h file (and become a
NOP) to prevent link errors.

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

The joys of static linking - once you need one function in a .o, all others in that file get linked in, along with their dependencies, even if they remain uncalled. I don't know if that means we need to refactor our qemu driver into yet more files, but that's a question for another day; I'm fine with the extra library in tests/Makefile.am for now.


Change from V1: rather than making things compile properly when
configured with --without-network by dropping #if WITH_NETWORK all
over in the .c files, in this version I just redefine the functions in
question (network.....()) as static inline in the .h file when
WITH_NETWORK is false:


This works perfectly, but "make syntax-check" doesn't like the fact
that I've put ATTRIBUTE_UNUSED on the args of the function
prototype. I think make syntax-check should be changed to allow
ATTRIBUTE_UNUSED in a .h file, as long as it's part of a function
definition (rather than declaration), but could be convinced
otherwise if needed to get this pushed sooner.

It would be easy enough to exempt just that one header from the syntax check rule (but don't apply this - see below):

diff --git i/cfg.mk w/cfg.mk
index f2a951d..651e7e8 100644
--- i/cfg.mk
+++ w/cfg.mk
@@ -664,6 +664,9 @@ $(srcdir)/src/remote/remote_client_bodies.h: $(srcdir)/src/remote/remote_protoco
 	$(MAKE) -C src remote/remote_client_bodies.h

 # List all syntax-check exemptions:
+exclude_file_name_regexp--sc_avoid_attribute_unused_in_header = \
+  ^src/network/bridge_driver\.h$$
 exclude_file_name_regexp--sc_avoid_strcase = ^tools/virsh\.c$$


+/* 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.
+ */
+networkAllocateActualDevice(virDomainNetDefPtr iface)
+    struct network_driver *driver = driverState;
+    virNetworkObjPtr network;
+    virNetworkDefPtr netdef;
+    int ret = -1;
+    if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK)

Uses iface without first checking for NULL, so let's mark that in the .h file.

+        if (virtport) {
+            if (VIR_ALLOC(iface->data.network.actual->data.direct.virtPortProfile)<  0) {
+                virReportOOMError();
+                goto cleanup;
+            }
+            /* There are no pointers in a virtualPortProfile, so a shallow copy
+             * is sufficient
+             */
+            *iface->data.network.actual->data.direct.virtPortProfile = *virtport;

Will this ever bite us later if we add a pointer to the struct, but forget to update this line of code? Adding a helper function now, even if the helper function just uses bitwise shallow copy for now, might ease maintenance down the road. But it's not worth holding up this patch just for that.

+        }
+        /* If there is only a single device, just return it (caller will detect
+         * any error if exclusive use is required but could be acquired).

s/could be/could not be/

  int networkRegister(void);

The cppi syntax-check didn't like this.

+int networkAllocateActualDevice(virDomainNetDefPtr iface);

See comment above about adding attributes.

+static inline int
+networkAllocateActualDevice(virDomainNetDefPtr iface ATTRIBUTE_UNUSED)

I don't know of any way to write a regex that avoids ATTRIBUTE_UNUSED but permits static inline.

However, it _is_ possible to write a macro rather than use a static inline function, at which point you don't need ATTRIBUTE_UNUSED in the first place!

ACK with this squashed in:

diff --git i/src/network/bridge_driver.c w/src/network/bridge_driver.c
index b36b1f1..99033a2 100644
--- i/src/network/bridge_driver.c
+++ w/src/network/bridge_driver.c
@@ -2838,7 +2838,7 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
*iface->data.network.actual->data.direct.virtPortProfile = *virtport;
/* If there is only a single device, just return it (caller will detect
-         * any error if exclusive use is required but could be acquired).
+ * any error if exclusive use is required but could not be acquired).
         if (netdef->nForwardIfs == 0) {
diff --git i/src/network/bridge_driver.h w/src/network/bridge_driver.h
index 710d862..d409f76 100644
--- i/src/network/bridge_driver.h
+++ w/src/network/bridge_driver.h
@@ -1,7 +1,7 @@
  * network_driver.h: core driver methods for managing networks
- * Copyright (C) 2006, 2007 Red Hat, Inc.
+ * Copyright (C) 2006, 2007, 2011 Red Hat, Inc.
  * Copyright (C) 2006 Daniel P. Berrange
  * This library is free software; you can redistribute it and/or
@@ -35,35 +35,25 @@

 int networkRegister(void);

-int networkAllocateActualDevice(virDomainNetDefPtr iface);
-int networkNotifyActualDevice(virDomainNetDefPtr iface);
-int networkReleaseActualDevice(virDomainNetDefPtr iface);
+int networkAllocateActualDevice(virDomainNetDefPtr iface)
+int networkNotifyActualDevice(virDomainNetDefPtr iface)
+int networkReleaseActualDevice(virDomainNetDefPtr iface)

 int networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network,
virCommandPtr *cmdout, char *pidfile,
-                                      dnsmasqContext *dctx);
-static inline int
-networkAllocateActualDevice(virDomainNetDefPtr iface ATTRIBUTE_UNUSED)
-{ return 0; }
-static inline int
-networkNotifyActualDevice(virDomainNetDefPtr iface ATTRIBUTE_UNUSED)
-{ return 0; }
-static inline int
-networkReleaseActualDevice(virDomainNetDefPtr iface ATTRIBUTE_UNUSED)
-{ return 0; }
-static inline int
-networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network ATTRIBUTE_UNUSED,
-                                  virCommandPtr *cmdout ATTRIBUTE_UNUSED,
-                                  char *pidfile ATTRIBUTE_UNUSED,
-                                  dnsmasqContext *dctx ATTRIBUTE_UNUSED)
-{ return 0; }
+                                      dnsmasqContext *dctx)
+    ;
+# else
+/* Define no-op replacements that don't drag in any link dependencies.  */
+#  define networkAllocateActualDevice(iface) 0
+#  define networkNotifyActualDevice(iface) 0
+#  define networkReleaseActualDevice(iface) 0
+# define networkBuildDhcpDaemonCommandLine(network, cmdout, pidfile, dctx) 0
+# endif

 typedef char *(*networkDnsmasqLeaseFileNameFunc)(const char *netname);

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

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