[libvirt] [PATCH 2/3] network: consolidated info log for all network allocate/free operations
Laine Stump
laine at laine.org
Sun Feb 14 16:21:40 UTC 2016
On 02/12/2016 04:39 PM, John Ferlan wrote:
>
> 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
Yikes! Don't know how I overlooked that. Thanks!
>
>> + 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).
Since nothing is allocated from the network when the hook fails, it
makes sense to not log the message in
that case. In the past we were actually *incorrectly* logging a message
stating that a new interface had been connected to the network when this
was in fact not true.
>
>> }
>>
>> 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...
Same response here, although this does point out that the hook run when
an interface is newly connected to a network is the same as the hook
that is run when libvirtd is restarted and we're just repopulating the
network status with existing allocations. I don't know if that's what's
wanted/expected or not, but most probably it *shouldn't* be that way
(consider, for a moment, a hook that is used for some sort of setup that
can only be done once - it would fail if the hook was re-run when
libvirtd was restarted). Instead we should probably have a different
hook state for the "restart libvirtd" case, similar to "reconnect" for
the qemu hook.
>>
>> 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...
There's nothing to be done if it fails, and the interface really is
disconnected whether or not the hook succeeds.
>
>> + networkLogAllocation(netdef, actualType, dev, iface, false);
> And this one prints regardless...
The resources are still freed regardless of whether or not the hook
fails, so the log message should still be logged.
>
> Your call on how to proceed - that is not printing if Hook fails is desired.
Well, virHookCall() does log an error in any case that it fails, so that
would be seen in the logs. It would probably be useful to log more
complete info in failure cases. The intent of this patch is to get the
info logs correct when an interface is successfully
connected/disconnected though - I think any beefing up of error logs can
be done separately.
>
> ACK as long as the connections++ is removed from networkLogAllocation
Done, and pushing. Thanks!
More information about the libvir-list
mailing list