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.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list