[libvirt] [PATCH 7/7] network: include plugged interface XML in "plugged" network hook

[Michal Privoznik]

On 21.02.2014 14:58, Laine Stump wrote:
> The network hook script gets called whenever an interface is plugged
> into or unplugged from a network, but even though the full XML of both
> the network and the domain is included, there is no reasonable way to
> determine what exact resources the plugged interface is using:
> 
> 1) Prior to a recent patch which modified the status XML of interfaces
> to include the information about actual hardware resources used, it
> would be possible to scan through the domain XML output sent to the
> hook, and from there find the correct interface, but that interface
> definition would not include any runtime info (e.g. bandwidth or vlan
> taken from a portgroup, or which physdev was used in case of a macvtap
> network).
> 
> 2) After the patch modifying the status XML of interfaces, the network
> name would no longer be included in the domain XML, so it would be
> completely impossible to determine which interface was the one being
> plugged.
> 
> To solve that problem, this patch includes a single <interface>
> element at the beginning of the XML sent to the network hook for
> "plugged" and "unplugged" (just inside <hookData>) that is the status
> XML of the interface being plugged. This XML will include all info
> gathered from the chosen network and portgroup.
> 
> NB: due to hardcoded spaces in all of the device *Format() functions,
> the <interface> element inside the <hookData> will be indented by 6
> spaces rather than 2. I had intended to fix this, but it turns out
> that to make virDomainNetDefFormat() indentation relative, I would
> have to do the same to virDomainDeviceInfoFormat(), and that function
> is called from 19 places - making that a prerequisite of this patch
> would cause too many merge difficulties if we needed to backport
> network hooks, so I chose to ignore the problem here and fix the
> problem for *all* devices in a followup later.
> ---
>   src/network/bridge_driver.c | 48 ++++++++++++++++++++++++++++++---------------
>   1 file changed, 32 insertions(+), 16 deletions(-)
> 
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index a6c719d..152bd06 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -141,6 +141,7 @@ networkObjFromNetwork(virNetworkPtr net)
>   static int
>   networkRunHook(virNetworkObjPtr network,
>                  virDomainDefPtr dom,
> +               virDomainNetDefPtr iface,
>                  int op,
>                  int sub_op)
>   {
> @@ -158,6 +159,8 @@ networkRunHook(virNetworkObjPtr network,
>   
>           virBufferAddLit(&buf, "<hookData>\n");
>           virBufferAdjustIndent(&buf, 2);
> +        if (iface && virDomainNetDefFormat(&buf, iface, 0) < 0)
> +            goto cleanup;
>           if (virNetworkDefFormatBuf(&buf, network->def, 0) < 0)
>               goto cleanup;
>           if (dom && virDomainDefFormatInternal(dom, 0, &buf) < 0)
> @@ -2067,7 +2070,7 @@ networkStartNetwork(virNetworkDriverStatePtr driver,
>   
>       /* Run an early hook to set-up missing devices.
>        * If the script raised an error abort the launch. */
> -    if (networkRunHook(network, NULL,
> +    if (networkRunHook(network, NULL, NULL,
>                          VIR_HOOK_NETWORK_OP_START,
>                          VIR_HOOK_SUBOP_BEGIN) < 0)
>           goto cleanup;
> @@ -2092,7 +2095,7 @@ networkStartNetwork(virNetworkDriverStatePtr driver,
>       }
>   
>       /* finally we can call the 'started' hook script if any */
> -    if (networkRunHook(network, NULL,
> +    if (networkRunHook(network, NULL, NULL,
>                          VIR_HOOK_NETWORK_OP_STARTED,
>                          VIR_HOOK_SUBOP_BEGIN) < 0)
>           goto cleanup;
> @@ -2158,7 +2161,7 @@ static int networkShutdownNetwork(virNetworkDriverStatePtr driver,
>       }
>   
>       /* now that we know it's stopped call the hook if present */
> -    networkRunHook(network, NULL, VIR_HOOK_NETWORK_OP_STOPPED,
> +    networkRunHook(network, NULL, NULL, VIR_HOOK_NETWORK_OP_STOPPED,
>                      VIR_HOOK_SUBOP_END);
>   
>       network->active = 0;
> @@ -3659,14 +3662,8 @@ validate:
>           }
>       }
>   
> -    /* finally we can call the 'plugged' hook script if any */
> -    if (networkRunHook(network, dom,
> -                       VIR_HOOK_NETWORK_OP_IFACE_PLUGGED,
> -                       VIR_HOOK_SUBOP_BEGIN) < 0)
> -        goto error;
> -
>       if (dev) {
> -        /* we are now assured of success, so mark the allocation */
> +        /* mark the allocation */
>           dev->connections++;
>           if (actualType != VIR_DOMAIN_NET_TYPE_HOSTDEV) {
>               VIR_DEBUG("Using physical device %s, %d connections",
> @@ -3684,6 +3681,19 @@ validate:
>           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) {
> +        /* adjust for failure */
> +        if (dev)
> +            dev->connections--;
> +        if (netdef)
> +            netdef->connections--;
> +        goto error;
> +    }
> +
>       ret = 0;
>   
>   cleanup:
> @@ -3865,14 +3875,20 @@ networkNotifyActualDevice(virDomainDefPtr dom,
>       }
>   
>   success:
> -    /* finally we can call the 'plugged' hook script if any */
> -    if (networkRunHook(network, dom, VIR_HOOK_NETWORK_OP_IFACE_PLUGGED,
> -                       VIR_HOOK_SUBOP_BEGIN) < 0)
> -        goto error;
> -
>       netdef->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) {
> +        /* adjust for failure */
> +        if (dev)
> +            dev->connections--;
> +        netdef->connections--;
> +        goto error;
> +    }
> +
>       ret = 0;
>   cleanup:
>       if (network)
> @@ -4018,7 +4034,7 @@ success:
>           netdef->connections--;
>   
>       /* finally we can call the 'unplugged' hook script if any */
> -    networkRunHook(network, dom, VIR_HOOK_NETWORK_OP_IFACE_UNPLUGGED,
> +    networkRunHook(network, dom, iface, VIR_HOOK_NETWORK_OP_IFACE_UNPLUGGED,
>                      VIR_HOOK_SUBOP_BEGIN);
>   
>       VIR_DEBUG("Releasing network %s, %d connections",
> 

ACK although the indentation of XML we're passing to the hook script seems off:

<hookData>
      <interface type='network'>
      ...
      </interface>
  <network>
      ...
  </network>
  <domain type='kvm'>
  </domain>
</hookData>

This is not a show stopper for me. I wonder if we should push these patches now, even during the freeze as this is very closely related to the network hooks - one of the main features in this release.

Michal