On 02/24/2014 06:45 PM, Michal Privoznik wrote:
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:
Right. That's what the last paragraph of the commit log message is about - fixing that indentation would require a much more invasive change that would touch all the other device parsing functions, which could turn any potential backport into a real headache, so I chose to not fix it in this series.
<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.
In a way it is a bugfix for that feature (since the functionality of the "plugged" hook is severely limited without it). I would feel more comfortable about pushing it, though, if danpb took a look at the commit log message for patch 5/7 and gave his okay on the change in the XML. My opinion is that I should have done it this way to begin with, but Dan has much better insight than I do on what is and isn't good for management applications.