[libvirt] [PATCHv2 9/9] network: internal API functions to manage assignment of physdev to guest
Eric Blake
eblake at redhat.com
Thu Jul 21 14:48:17 UTC 2011
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:
>
> https://www.redhat.com/archives/libvir-list/2011-July/msg00534.html
>
> 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$$
_src1=libvirt|fdstream|qemu/qemu_monitor|util/(command|util)|xen/xend_internal|rpc/virnetsocket
> +
> +/* 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)
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);
> +
> +#if WITH_NETWORK
The cppi syntax-check didn't like this.
> +int networkAllocateActualDevice(virDomainNetDefPtr iface);
See comment above about adding attributes.
> +#else
> +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) {
networkReportError(VIR_ERR_INTERNAL_ERROR,
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);
-#if WITH_NETWORK
-int networkAllocateActualDevice(virDomainNetDefPtr iface);
-int networkNotifyActualDevice(virDomainNetDefPtr iface);
-int networkReleaseActualDevice(virDomainNetDefPtr iface);
+# if WITH_NETWORK
+int networkAllocateActualDevice(virDomainNetDefPtr iface)
+ ATTRIBUTE_NONNULL(1);
+int networkNotifyActualDevice(virDomainNetDefPtr iface)
+ ATTRIBUTE_NONNULL(1);
+int networkReleaseActualDevice(virDomainNetDefPtr iface)
+ ATTRIBUTE_NONNULL(1);
int networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network,
virCommandPtr *cmdout, char
*pidfile,
- dnsmasqContext *dctx);
-#else
-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; }
-
-#endif
+ 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 at redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
More information about the libvir-list
mailing list