[libvirt] [PATCH 2/3] network: consolidated info log for all network allocate/free operations
John Ferlan
jferlan at redhat.com
Fri Feb 12 21:39:04 UTC 2016
On 02/11/2016 04:37 PM, Laine Stump wrote:
> There are three functions that deal with allocating and freeing
> devices from a networks netdev/pci device pool:
> network(Allocate|Notify|Release)ActualDevice(). These functions also
> maintain a counter of the number of domains currently using a network
> (regardless of whether or not that network uses a device pool). Each
> of these functions had multiple log messages (output using VIR_DEBUG)
> that were in slightly different formats and gave varying amounts of
> information.
>
> This patch creates a single function to log the pertinent information
> in a consistent manner for all three of these functions. Along with
> assuring that all the functions produce a consistent form of output
> (and making it simpler to change), it adds the MAC address of the
> domain interface involved in the operation, making it possible to
> verify which interface of which domain the operation is being done for
> (assuming that all MAC addresses are unique, of course).
>
> All of these messages are raised from DEBUG to INFO, since they don't
> happen that often (once per interface per domain/libvirtd start or
> domain stop), and can be very informative and helpful - eliminating
> the need to log debug level messages makes it much easier to sort
> these out.
> ---
> src/network/bridge_driver.c | 80 ++++++++++++++++++++++-----------------------
> 1 file changed, 39 insertions(+), 41 deletions(-)
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index dcd38ed..c305f8e 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -3863,6 +3863,40 @@ int networkRegister(void)
> * driver, we will need to present these functions via a second
> * "backend" function table.
> */
^^^ looks like the above error message needs to move below this
function. You could add a quick comment here, too
> +static void
> +networkLogAllocation(virNetworkDefPtr netdef,
> + virDomainNetType actualType,
> + virNetworkForwardIfDefPtr dev,
> + virDomainNetDefPtr iface,
> + bool inUse)
> +{
> + char macStr[VIR_MAC_STRING_BUFLEN];
> + const char *verb = inUse ? "using" : "releasing";
> +
> + if (!dev) {
> + VIR_INFO("MAC %s %s network %s (%d connections)",
> + virMacAddrFormat(&iface->mac, macStr), verb,
> + netdef->name, netdef->connections);
> + } else {
> + /* mark the allocation */
> + dev->connections++;
^^ Isn't this a duplicate [1]? Probably don't want these two lines
> + if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
> + VIR_INFO("MAC %s %s network %s (%d connections) "
> + "physical device %04x:%02x:%02x.%x (%d connections)",
> + virMacAddrFormat(&iface->mac, macStr), verb,
> + netdef->name, netdef->connections,
> + dev->device.pci.domain, dev->device.pci.bus,
> + dev->device.pci.slot, dev->device.pci.function,
> + dev->connections);
> + } else {
> + VIR_INFO("MAC %s %s network %s (%d connections) "
> + "physical device %s (%d connections)",
> + virMacAddrFormat(&iface->mac, macStr), verb,
> + netdef->name, netdef->connections,
> + dev->device.dev, dev->connections);
> + }
> + }
> +}
>
> /* networkAllocateActualDevice:
> * @dom: domain definition that @iface belongs to
> @@ -4236,23 +4270,8 @@ networkAllocateActualDevice(virDomainDefPtr dom,
>
> if (netdef) {
> netdef->connections++;
> - VIR_DEBUG("Using network %s, %d connections",
> - netdef->name, netdef->connections);
> -
> - if (dev) {
> - /* mark the allocation */
> + if (dev)
> dev->connections++;
[1] ?
> - if (actualType != VIR_DOMAIN_NET_TYPE_HOSTDEV) {
> - VIR_DEBUG("Using physical device %s, %d connections",
> - dev->device.dev, dev->connections);
> - } else {
> - VIR_DEBUG("Using physical device %04x:%02x:%02x.%x, connections %d",
> - dev->device.pci.domain, dev->device.pci.bus,
> - dev->device.pci.slot, dev->device.pci.function,
> - dev->connections);
> - }
> - }
> -
> /* finally we can call the 'plugged' hook script if any */
> if (networkRunHook(network, dom, iface,
> VIR_HOOK_NETWORK_OP_IFACE_PLUGGED,
> @@ -4263,6 +4282,7 @@ networkAllocateActualDevice(virDomainDefPtr dom,
> dev->connections--;
> goto error;
> }
> + networkLogAllocation(netdef, actualType, dev, iface, true);
If the Hook fails, then there's no messages printed... Whether that
matters (they used to be regardless).
> }
>
> ret = 0;
> @@ -4388,10 +4408,6 @@ networkNotifyActualDevice(virDomainDefPtr dom,
> netdef->name, actualDev);
> goto error;
> }
> -
> - VIR_DEBUG("Using physical device %s, connections %d",
> - dev->device.dev, dev->connections + 1);
> -
> } else /* if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) */ {
> virDomainHostdevDefPtr hostdev;
>
> @@ -4441,20 +4457,12 @@ networkNotifyActualDevice(virDomainDefPtr dom,
> dev->device.pci.slot, dev->device.pci.function);
> goto error;
> }
> -
> - VIR_DEBUG("Using physical device %04x:%02x:%02x.%x, connections %d",
> - dev->device.pci.domain, dev->device.pci.bus,
> - dev->device.pci.slot, dev->device.pci.function,
> - dev->connections);
> }
>
> success:
> netdef->connections++;
> if (dev)
> dev->connections++;
> - VIR_DEBUG("Using network %s, %d connections",
> - netdef->name, netdef->connections);
> -
> /* finally we can call the 'plugged' hook script if any */
> if (networkRunHook(network, dom, iface, VIR_HOOK_NETWORK_OP_IFACE_PLUGGED,
> VIR_HOOK_SUBOP_BEGIN) < 0) {
> @@ -4464,6 +4472,7 @@ networkNotifyActualDevice(virDomainDefPtr dom,
> netdef->connections--;
> goto error;
> }
> + networkLogAllocation(netdef, actualType, dev, iface, true);
Another that may not print if Hook fails...
>
> ret = 0;
> cleanup:
> @@ -4475,6 +4484,7 @@ networkNotifyActualDevice(virDomainDefPtr dom,
> }
>
>
> +
> /* networkReleaseActualDevice:
> * @dom: domain definition that @iface belongs to
> * @iface: a domain's NetDef (interface definition)
> @@ -4559,10 +4569,6 @@ networkReleaseActualDevice(virDomainDefPtr dom,
> netdef->name, actualDev);
> goto error;
> }
> -
> - VIR_DEBUG("Releasing physical device %s, connections %d",
> - dev->device.dev, dev->connections - 1);
> -
> } else /* if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) */ {
> virDomainHostdevDefPtr hostdev;
>
> @@ -4594,11 +4600,6 @@ networkReleaseActualDevice(virDomainDefPtr dom,
> hostdev->source.subsys.u.pci.addr.function);
> goto error;
> }
> -
> - VIR_DEBUG("Releasing physical device %04x:%02x:%02x.%x, connections %d",
> - dev->device.pci.domain, dev->device.pci.bus,
> - dev->device.pci.slot, dev->device.pci.function,
> - dev->connections - 1);
> }
>
> success:
> @@ -4606,13 +4607,10 @@ networkReleaseActualDevice(virDomainDefPtr dom,
> netdef->connections--;
> if (dev)
> dev->connections--;
> -
> - VIR_DEBUG("Releasing network %s, %d connections",
> - netdef->name, netdef->connections);
> -
> /* finally we can call the 'unplugged' hook script if any */
> networkRunHook(network, dom, iface, VIR_HOOK_NETWORK_OP_IFACE_UNPLUGGED,
> VIR_HOOK_SUBOP_BEGIN);
Existing... no status check like there are on other calls...
> + networkLogAllocation(netdef, actualType, dev, iface, false);
And this one prints regardless...
Your call on how to proceed - that is not printing if Hook fails is desired.
ACK as long as the connections++ is removed from networkLogAllocation
John
> }
> ret = 0;
> cleanup:
>
More information about the libvir-list
mailing list