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

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.