Re: [PATCH 2/3] network: consolidated info log for all network allocate/free operations

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

 




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:
> 

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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]