On 02/05/2014 12:11 PM, Michal Privoznik wrote:

The lack of debug printings might be frustrating in the future. Moreover, this function doesn't follow the usual pattern we have in the rest of the code:

int ret = -1; /* do some work */ ret = 0; cleanup: /* some cleanup work */ return ret;

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index c8b167b..2bd015b 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1992,23 +1992,29 @@ static int networkStartNetwork(virNetworkDriverStatePtr driver, virNetworkObjPtr network) { - int ret = 0; + int ret = -1; + + VIR_DEBUG("driver=%p, network=%p", driver, network);

if (virNetworkObjIsActive(network)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("network is already active")); - return -1; + return ret;

Okay, this fixes the one problem I saw with v1 (calling UnsetDefTransient on cleanup for a network that was already active).

}

+ VIR_DEBUG("Beginning network startup process"); + + VIR_DEBUG("Setting current network def as transient"); if (virNetworkObjSetDefTransient(network, true) < 0) - return -1; + goto cleanup;

I *think* it's safe to call UnsetDefTransient when SetDefTransient failed, so this is probably okay. And the rest of cleanup (which is mostly just a call to networkShutdownNetwork(), after saving any errors) is safe as well - it's a NOP if the network isn't active. ACK.

switch (network->def->forward.type) {

case VIR_NETWORK_FORWARD_NONE: case VIR_NETWORK_FORWARD_NAT: case VIR_NETWORK_FORWARD_ROUTE: - ret = networkStartNetworkVirtual(driver, network); + if (networkStartNetworkVirtual(driver, network) < 0) + goto cleanup; break;

case VIR_NETWORK_FORWARD_BRIDGE: @@ -2016,28 +2022,25 @@ networkStartNetwork(virNetworkDriverStatePtr driver, case VIR_NETWORK_FORWARD_VEPA: case VIR_NETWORK_FORWARD_PASSTHROUGH: case VIR_NETWORK_FORWARD_HOSTDEV: - ret = networkStartNetworkExternal(driver, network); + if (networkStartNetworkExternal(driver, network) < 0) + goto cleanup; break; }

- if (ret < 0) { - virNetworkObjUnsetDefTransient(network); - return ret; - } - /* Persist the live configuration now that anything autogenerated * is setup. */ - if ((ret = virNetworkSaveStatus(driverState->stateDir, - network)) < 0) { - goto error; - } + VIR_DEBUG("Writing network status to disk"); + if (virNetworkSaveStatus(driverState->stateDir, network) < 0) + goto cleanup;

- VIR_INFO("Starting up network '%s'", network->def->name); network->active = 1; + VIR_INFO("Network '%s' started up", network->def->name); + ret = 0;

-error: +cleanup: if (ret < 0) { + virNetworkObjUnsetDefTransient(network); virErrorPtr save_err = virSaveLastError(); int save_errno = errno; networkShutdownNetwork(driver, network);

On 02/05/2014 12:11 PM, Michal Privoznik wrote:

There might be some use cases, where user wants to prepare the host or its environment prior to starting a network and do some cleanup after the network has been shut down. Consider all the functionality that libvirt doesn't currently have as an example what a hook script can possibly do.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/hooks.html.in | 60 ++++++++++++++++++---- src/lxc/lxc_driver.c | 4 +- src/lxc/lxc_process.c | 6 +-- src/network/bridge_driver.c | 120 +++++++++++++++++++++++++++++++++++++++++++- src/network/bridge_driver.h | 14 +++--- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_hotplug.c | 14 +++--- src/qemu/qemu_process.c | 2 +- src/util/virhook.c | 13 ++++- src/util/virhook.h | 11 ++++ 10 files changed, 212 insertions(+), 34 deletions(-)

diff --git a/docs/hooks.html.in b/docs/hooks.html.in index f0f692b..9deb215 100644 --- a/docs/hooks.html.in +++ b/docs/hooks.html.in @@ -16,6 +16,9 @@ configuration<br/><br/></li> <li>A QEMU guest is started or stopped<br/><br/></li> <li>An LXC guest is started or stopped<br/><br/></li>

Do we know when support for the above hooks was added? The way it's written here, it sounds like support for *all* hook types was added in 1.2.2. If the support has been there since those particular hypervisor drivers were added, then we should put that version.

+ <li>A network is started or stopped or an interface is + un-/plugged from/to the network + (<span class="since">since 1.2.2</span>)<br/><br/></li> </ul>

<h2><a name="location">Script location</a></h2> @@ -44,6 +47,9 @@ Executed when a QEMU guest is started, stopped, or migrated<br/><br/></li> <li><code>/etc/libvirt/hooks/lxc</code><br /><br/> Executed when an LXC guest is started or stopped</li> + <li><code>/etc/libvirt/hooks/network</code><br/><br/> + Executed when a network is started or stopped or an + interface is un-/plugged from/to the network</li> </ul> <br/>

@@ -66,6 +72,12 @@ XML description for the domain on their stdin. This includes items such the UUID of the domain and its storage information, and is intended to provide all the libvirt information the script needs.</p> + <p>The network hook script is fed on its stdin with <b>full</b> XML + description of network in which an action occurs. However, in cases of + interface being plugged or unplugged to the network, script can expect + full XML description of corresponding domain appended after the network + XML (this happens when a domain with an <interface/> belonging to + the network is started or destroyed).</p>

How about this: For all cases, stdin of the network hook script is provided with the full XML description of the network status. In the case of an interface being plugged/unplugged to the network, the network XML will be followed with the full XML description of the domain containing the interface that is being plugged/unplugged.

<p>The command line arguments take this approach:</p> <ol> @@ -181,23 +193,49 @@ <pre>/etc/libvirt/hooks/lxc guest_name reconnect begin -</pre> </li> </ul> + + <h5><a name="network">/etc/libvirt/hooks/network</a></h5> + <ul> + <li><span class="since">Since 1.2.2</span>, before a network is started, + this script is called as:<br/> + <pre>/etc/libvirt/hooks/network network_name start begin -</pre></li> + <li>After the network is started, up ∧ running, the script is + called as:<br/> + <pre>/etc/libvirt/hooks/network network_name started begin -</pre></li> + <li>When a network is shut down, this script is called as:<br/> + <pre>/etc/libvirt/hooks/network network_name stopped end -</pre></li>

I wonder if anyone will ever need a "stop" event to be run just before we begin shutting down a network. I'm undecided if it's better to have it there from the beginning even if it's never used, or to not clutter the code until we come up with a reason for it.

+ <li>Later, when network is started and there's an interface from a + domain to be plugged into the network (plugged may not be the correct + expression when it comes to bridgeless netowrks, perhaps allocated is

s/netowrks/networks/ If we want to be completely accurate about what's happening under the covers, then I think you are correct about using allocate/free rather than plug/unplug. I don't know if that level of accuracy is all that important here though. Again, undecided.

+ better one then), the hook script is called as:<br/> + <pre>/etc/libvirt/hooks/network network_name plugged begin -</pre> + Please note, that in this case, the script is passed both network and + domain XMLs on its stdin.</li> + <li>When the domain from previous case is shutting down, the interface + is unplugged. This leads to another script invocation:<br/> + <pre>/etc/libvirt/hooks/network network_name unplugged begin -</pre> + And again, as in previous case, both network and domain XMLs are passed + onto script's stdin.</li>

Again, I'm wondering if anyone will ever be concerned about whether their hook is executed before or after the network device is allocated/created/connected, and likewise before/after freeing/destroying/unplugging/whatever you want to call it.

+ </ul> + <br/>

<h2><a name="execution">Script execution</a></h2> <ul> - <li>The "start" operation for the guest hook scripts, qemu and lxc, - executes <b>prior</b> to the guest being created. This allows the - guest start operation to be aborted if the script returns indicating - failure.<br/><br/></li> - <li>The "shutdown" operation for the guest hook scripts, qemu and lxc, - executes <b>after</b> the guest has stopped. If the hook script - indicates failure in its return, the shut down of the guest cannot - be aborted because it has already been performed.<br/><br/></li> + <li>The "start" operation for the guest and network hook scripts, + executes <b>prior</b> to the object (guest or network) being created. + This allows the object start operation to be aborted if the script + returns indicating failure.<br/><br/></li> + <li>The "shutdown" operation for the guest and network hook scripts, + executes <b>after</b> the object (guest or network) has stopped. If + the hook script indicates failure in its return, the shut down of the + object cannot be aborted because it has already been performed. + <br/><br/></li> <li>Hook scripts execute in a synchronous fashion. Libvirt waits for them to return before continuing the given operation.<br/><br/> - This is most noticeable with the guest start operation, as a lengthy - operation in the hook script can mean an extended wait for the guest - to be available to end users.<br/><br/></li> + This is most noticeable with the guest or network start operation, + as a lengthy operation in the hook script can mean an extended wait + for the guest or network to be available to end users.<br/><br/></li> <li>For a hook script to be utilised, it must have its execute bit set (ie. chmod o+rx <i>qemu</i>), and must be present when the libvirt

(I know this is pre-existing text, but the usage of "ie." above is incorrect. It's an example of how to set the execute bit, not the exact command that would be used in all cases (since the script name could change), so it's more appropriate to use "e.g." instead)

daemon is started.<br/><br/></li> diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 138c706..c1a7605 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -3774,7 +3774,7 @@ lxcDomainAttachDeviceNetLive(virConnectPtr conn, * network's pool of devices, or resolve bridge device name * to the one defined in the network definition. */ - if (networkAllocateActualDevice(net) < 0) + if (networkAllocateActualDevice(vm->def, net) < 0)

On one hand I am dismayed to see this (even though I totally understand why you're doing it). On the other hand, I'm happy to see it. Explanation: When I wrote networkAllocateActualDevice() I had tried to keep the arglist as bare bones as possible, to make it easier if/when we later move the network driver into a separate daemon so that unprivileged libvirtd processes could have access to full networking capabilities. That's why I'm dismayed - it means that there will now be more stuff that will need to be included in a formal API and shipped between processes. On the other hand, having all that information available to networkAllocateActualDevice will mean that we can do things like keep track in the network status of which domains have interfaces connected to a particular network.

return -1;

actualType = virDomainNetGetActualType(net); @@ -4429,7 +4429,7 @@ lxcDomainDetachDeviceNetLive(virDomainObjPtr vm, ret = 0; cleanup: if (!ret) { - networkReleaseActualDevice(detach); + networkReleaseActualDevice(vm->def, detach); virDomainNetRemove(vm->def, detachidx); virDomainNetDefFree(detach); } diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index ed729f6..8989245 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -198,7 +198,7 @@ static void virLXCProcessCleanup(virLXCDriverPtr driver, iface->ifname)); ignore_value(virNetDevVethDelete(iface->ifname)); } - networkReleaseActualDevice(iface); + networkReleaseActualDevice(vm->def, iface); }

virDomainConfVMNWFilterTeardown(vm); @@ -374,7 +374,7 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn, * network's pool of devices, or resolve bridge device name * to the one defined in the network definition. */ - if (networkAllocateActualDevice(def->nets[i]) < 0) + if (networkAllocateActualDevice(def, def->nets[i]) < 0) goto cleanup;

if (VIR_EXPAND_N(*veths, *nveths, 1) < 0) @@ -476,7 +476,7 @@ cleanup: ignore_value(virNetDevOpenvswitchRemovePort( virDomainNetGetActualBridgeName(iface), iface->ifname)); - networkReleaseActualDevice(iface); + networkReleaseActualDevice(def, iface); } } return ret; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 2bd015b..1ba2b2d 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -71,6 +71,7 @@ #include "virstring.h" #include "viraccessapicheck.h" #include "network_event.h" +#include "virhook.h"

#define VIR_FROM_THIS VIR_FROM_NETWORK

@@ -2008,6 +2009,23 @@ networkStartNetwork(virNetworkDriverStatePtr driver, if (virNetworkObjSetDefTransient(network, true) < 0) goto cleanup;

+ /* Run an early hook to set-up missing devices */ + if (virHookPresent(VIR_HOOK_DRIVER_NETWORK)) { + char *xml = virNetworkDefFormat(network->def, 0);

yep, correct flags - this says to give the current status, not "inactive" config.

+ int hookret; + + hookret = virHookCall(VIR_HOOK_DRIVER_NETWORK, network->def->name, + VIR_HOOK_NETWORK_OP_START, VIR_HOOK_SUBOP_BEGIN, + NULL, xml, NULL); + VIR_FREE(xml); + + /* + * If the script raised an error abort the launch + */ + if (hookret < 0) + goto cleanup; + } + switch (network->def->forward.type) {

case VIR_NETWORK_FORWARD_NONE: @@ -2034,6 +2052,23 @@ networkStartNetwork(virNetworkDriverStatePtr driver, if (virNetworkSaveStatus(driverState->stateDir, network) < 0) goto cleanup;

+ /* finally we can call the 'started' hook script if any */ + if (virHookPresent(VIR_HOOK_DRIVER_NETWORK)) { + char *xml = virNetworkDefFormat(network->def, 0); + int hookret;

I was going to suggest formatting this once and keeping it around to make things faster, but then realized that the network status will change between the first time its formatted and now, so it's proper (even necessary) to recreate it. One thing I've noticed though is that you're not checking for non-null return from virNetworkDefFormat (here or in other places), and virHookCall itself also doesn't treat this as an error. So some problem formatting the network XML would lead to the script being executed with no network xml on stdin, rather than an error. I wonder if that should be an error at this level, or if virHookCall should *always* have a non-null 6th argument.

+ + hookret = virHookCall(VIR_HOOK_DRIVER_NETWORK, network->def->name, + VIR_HOOK_NETWORK_OP_STARTED, + VIR_HOOK_SUBOP_BEGIN, NULL, xml, NULL); + VIR_FREE(xml); + + /* + * If the script raised an error abort the launch + */ + if (hookret < 0) + goto cleanup; + } + network->active = 1; VIR_INFO("Network '%s' started up", network->def->name); ret = 0; @@ -2087,6 +2122,17 @@ static int networkShutdownNetwork(virNetworkDriverStatePtr driver, break; }

+ /* now that we know it's stopped call the hook if present */ + if (virHookPresent(VIR_HOOK_DRIVER_NETWORK)) { + char *xml = virNetworkDefFormat(network->def, 0); + + /* we can't stop the operation even if the script raised an error */ + virHookCall(VIR_HOOK_DRIVER_NETWORK, network->def->name, + VIR_HOOK_NETWORK_OP_STOPPED, VIR_HOOK_SUBOP_END, + NULL, xml, NULL); + VIR_FREE(xml); + } + network->active = 0; virNetworkObjUnsetDefTransient(network); return ret; @@ -3223,6 +3269,7 @@ finish: }

/* networkAllocateActualDevice: + * @dom: domain definition that @iface belongs to * @iface: the original NetDef from the domain * * Looks up the network reference by iface, allocates a physical @@ -3234,7 +3281,8 @@ finish: * Returns 0 on success, -1 on failure. */ int -networkAllocateActualDevice(virDomainNetDefPtr iface) +networkAllocateActualDevice(virDomainDefPtr dom, + virDomainNetDefPtr iface) { virNetworkDriverStatePtr driver = driverState; enum virDomainNetType actualType = iface->type; @@ -3567,6 +3615,42 @@ validate: } }

+ /* finally we can call the 'plugged' hook script if any */ + if (virHookPresent(VIR_HOOK_DRIVER_NETWORK)) { + /* the XML construction is a bit complicated here, + * as we want to pass both domain XML and network XML */ + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *xml, *net_xml, *dom_xml; + int hookret; + + net_xml = virNetworkDefFormat(netdef, 0); + dom_xml = virDomainDefFormat(dom, 0); + + virBufferAdd(&buf, net_xml, -1); + virBufferAdd(&buf, dom_xml, -1); + + VIR_FREE(net_xml); + VIR_FREE(dom_xml); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + goto error; + } + + xml = virBufferContentAndReset(&buf); + + hookret = virHookCall(VIR_HOOK_DRIVER_NETWORK, network->def->name, + VIR_HOOK_NETWORK_OP_IFACE_PLUGGED, + VIR_HOOK_SUBOP_BEGIN, NULL, xml, NULL); + VIR_FREE(xml); + + /* + * If the script raised an error abort the allocation + */ + if (hookret < 0) + goto error; + } + if (dev) { /* we are now assured of success, so mark the allocation */ dev->connections++; @@ -3780,6 +3864,7 @@ error:

/* networkReleaseActualDevice: + * @dom: domain definition that @iface belongs to * @iface: a domain's NetDef (interface definition) * * Given a domain <interface> element that previously had its <actual> @@ -3790,7 +3875,8 @@ error: * Returns 0 on success, -1 on failure. */ int -networkReleaseActualDevice(virDomainNetDefPtr iface) +networkReleaseActualDevice(virDomainDefPtr dom, + virDomainNetDefPtr iface) { virNetworkDriverStatePtr driver = driverState; enum virDomainNetType actualType = virDomainNetGetActualType(iface); @@ -3909,6 +3995,36 @@ networkReleaseActualDevice(virDomainNetDefPtr iface) success: if (iface->data.network.actual) netdef->connections--; + + /* finally we can call the 'unplugged' hook script if any */ + if (virHookPresent(VIR_HOOK_DRIVER_NETWORK)) { + /* the XML construction is a bit complicated here, + * as we want to pass both domain XML and network XML */ + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *xml, *net_xml, *dom_xml; + + net_xml = virNetworkDefFormat(netdef, 0); + dom_xml = virDomainDefFormat(dom, 0); + + virBufferAdd(&buf, net_xml, -1); + virBufferAdd(&buf, dom_xml, -1); + + VIR_FREE(net_xml); + VIR_FREE(dom_xml); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + goto error; + } + + xml = virBufferContentAndReset(&buf); + + virHookCall(VIR_HOOK_DRIVER_NETWORK, network->def->name, + VIR_HOOK_NETWORK_OP_IFACE_UNPLUGGED, + VIR_HOOK_SUBOP_BEGIN, NULL, xml, NULL); + VIR_FREE(xml); + } + VIR_DEBUG("Releasing network %s, %d connections", netdef->name, netdef->connections); ret = 0; diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h index 50258b5..e6640ac 100644 --- a/src/network/bridge_driver.h +++ b/src/network/bridge_driver.h @@ -34,12 +34,14 @@ int networkRegister(void);

# if WITH_NETWORK -int networkAllocateActualDevice(virDomainNetDefPtr iface) - ATTRIBUTE_NONNULL(1); +int networkAllocateActualDevice(virDomainDefPtr dom, + virDomainNetDefPtr iface) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int networkNotifyActualDevice(virDomainNetDefPtr iface) ATTRIBUTE_NONNULL(1);

Oh, right! The Notify function. This function is called for each interface connected to a libvirt network whenever libvirtd is restarted. Right now it's used to verify that the resources used by the guest are not being used by someone else, and to mark them as used in the newly-reconstructed network object. We are going to need an event here so that any iptables rules created during the "plugged" hook can be recreated - necessary because libvirtd reloads all of a network's iptables rules every time it is restarted (an example of why this is necessary is illustrated in a hackish use of the qemu hook here: http://wiki.libvirt.org/page/Networking#Forwarding_Incoming_Connections (We should re-write this hook as a network hook script once your patches are pushed).

-int networkReleaseActualDevice(virDomainNetDefPtr iface) - ATTRIBUTE_NONNULL(1); +int networkReleaseActualDevice(virDomainDefPtr dom, + virDomainNetDefPtr iface) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);

int networkGetNetworkAddress(const char *netname, char **netaddr) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); @@ -51,9 +53,9 @@ int networkDnsmasqConfContents(virNetworkObjPtr network, dnsmasqCapsPtr caps); # else /* Define no-op replacements that don't drag in any link dependencies. */ -# define networkAllocateActualDevice(iface) 0 +# define networkAllocateActualDevice(dom, iface) 0 # define networkNotifyActualDevice(iface) (iface=iface, 0) -# define networkReleaseActualDevice(iface) (iface=iface, 0) +# define networkReleaseActualDevice(dom, iface) (dom=dom, iface=iface, 0) # define networkGetNetworkAddress(netname, netaddr) (-2) # define networkDnsmasqConfContents(network, pidfile, configstr, \ dctx, caps) 0 diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 5b94de1..84a1c52 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -548,7 +548,7 @@ qemuNetworkPrepareDevices(virDomainDefPtr def) * network's pool of devices, or resolve bridge device name * to the one defined in the network definition. */ - if (networkAllocateActualDevice(net) < 0) + if (networkAllocateActualDevice(def, net) < 0) goto cleanup;

actualType = virDomainNetGetActualType(net); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 4a885d2..60fd88e 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -837,7 +837,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, * network's pool of devices, or resolve bridge device name * to the one defined in the network definition. */ - if (networkAllocateActualDevice(net) < 0) + if (networkAllocateActualDevice(vm->def, net) < 0) goto cleanup;

actualType = virDomainNetGetActualType(net); @@ -1082,7 +1082,7 @@ cleanup:

virDomainNetRemoveHostdev(vm->def, net);

- networkReleaseActualDevice(net); + networkReleaseActualDevice(vm->def, net); }

VIR_FREE(nicstr); @@ -2017,7 +2017,7 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, * free it if we fail for any reason */ if (newdev->type == VIR_DOMAIN_NET_TYPE_NETWORK && - networkAllocateActualDevice(newdev) < 0) { + networkAllocateActualDevice(vm->def, newdev) < 0) { goto cleanup; }

@@ -2204,7 +2204,7 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,

/* this function doesn't work with HOSTDEV networks yet, thus * no need to change the pointer in the hostdev structure */ - networkReleaseActualDevice(olddev); + networkReleaseActualDevice(vm->def, olddev); virDomainNetDefFree(olddev); /* move newdev into the nets list, and NULL it out from the * virDomainDeviceDef that we were given so that the caller @@ -2236,7 +2236,7 @@ cleanup: * replace the entire device object. */ if (newdev) - networkReleaseActualDevice(newdev); + networkReleaseActualDevice(vm->def, newdev);

return ret; } @@ -2649,7 +2649,7 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, virDomainHostdevDefFree(hostdev);

if (net) { - networkReleaseActualDevice(net); + networkReleaseActualDevice(vm->def, net); virDomainNetDefFree(net); } virObjectUnref(cfg); @@ -2717,7 +2717,7 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, virDomainNetGetActualBridgeName(net), net->ifname));

- networkReleaseActualDevice(net); + networkReleaseActualDevice(vm->def, net); virDomainNetDefFree(net); virObjectUnref(cfg); } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 8bcd98e..8b1d7a7 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4393,7 +4393,7 @@ void qemuProcessStop(virQEMUDriverPtr driver,

/* kick the device out of the hostdev list too */ virDomainNetRemoveHostdev(def, net); - networkReleaseActualDevice(net); + networkReleaseActualDevice(vm->def, net); }

retry: diff --git a/src/util/virhook.c b/src/util/virhook.c index 159efdb..0f5d0c5 100644 --- a/src/util/virhook.c +++ b/src/util/virhook.c @@ -48,12 +48,14 @@ VIR_ENUM_DECL(virHookDaemonOp) VIR_ENUM_DECL(virHookSubop) VIR_ENUM_DECL(virHookQemuOp) VIR_ENUM_DECL(virHookLxcOp) +VIR_ENUM_DECL(virHookNetworkOp)

VIR_ENUM_IMPL(virHookDriver, VIR_HOOK_DRIVER_LAST, "daemon", "qemu", - "lxc") + "lxc", + "network")

VIR_ENUM_IMPL(virHookDaemonOp, VIR_HOOK_DAEMON_OP_LAST, "start", @@ -83,6 +85,13 @@ VIR_ENUM_IMPL(virHookLxcOp, VIR_HOOK_LXC_OP_LAST, "started", "reconnect")

+VIR_ENUM_IMPL(virHookNetworkOp, VIR_HOOK_NETWORK_OP_LAST, + "start", + "started", + "stopped", + "plugged", + "unplugged") + static int virHooksFound = -1;

/** @@ -246,6 +255,8 @@ virHookCall(int driver, case VIR_HOOK_DRIVER_LXC: opstr = virHookLxcOpTypeToString(op); break; + case VIR_HOOK_DRIVER_NETWORK: + opstr = virHookNetworkOpTypeToString(op); } if (opstr == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/util/virhook.h b/src/util/virhook.h index 96bf4cf..7b09ac5 100644 --- a/src/util/virhook.h +++ b/src/util/virhook.h @@ -30,6 +30,7 @@ enum virHookDriverType { VIR_HOOK_DRIVER_DAEMON = 0, /* Daemon related events */ VIR_HOOK_DRIVER_QEMU, /* QEmu domains related events */ VIR_HOOK_DRIVER_LXC, /* LXC domains related events */ + VIR_HOOK_DRIVER_NETWORK, /* network related events */

VIR_HOOK_DRIVER_LAST, }; @@ -74,6 +75,16 @@ enum virHookLxcOpType { VIR_HOOK_LXC_OP_LAST, };

+enum virHookNetworkOpType { + VIR_HOOK_NETWORK_OP_START, /* network is about to start */ + VIR_HOOK_NETWORK_OP_STARTED, /* network has start */ + VIR_HOOK_NETWORK_OP_STOPPED, /* network has stopped */ + VIR_HOOK_NETWORK_OP_IFACE_PLUGGED, /* an interface has been plugged into the network */ + VIR_HOOK_NETWORK_OP_IFACE_UNPLUGGED, /* an interface was unplugged from the network */ + + VIR_HOOK_NETWORK_OP_LAST, +}; + int virHookInitialize(void);

int virHookPresent(int driver);

----- Original Message -----

From: "Laine Stump" <laine@laine.org> To: libvir-list@redhat.com Cc: "Michal Privoznik" <mprivozn@redhat.com> Sent: Friday, February 7, 2014 1:17:10 PM Subject: Re: [libvirt] [PATCH v2 3/3] network: Taint networks that are using hook script

On 02/05/2014 12:11 PM, Michal Privoznik wrote:

...

Basically, the idea is copied from domain code, where tainting exists for a while. Currently, only one taint reason exists - VIR_NETWORK_TAINT_HOOK to mark those networks which caused invoking of hook script.

What's missing here is that the network status XML doesn't include a <taint> element.

Also, I think if a network is tainted, and domain that connects to that network should be tainted as well.

Of course what would make this more useful would be if would could determine when a hook script actually *did* something for a particular network/interface (since presumably people are usually going to write their network hook scripts to only take action for particular networks and/or domains, not for *all* networks). I don't know that there's a way to do that without either 1) having a different hook script for each network, or 2) trusting the hook script to return some sort of status indicating whether or not it did anything. Obviously (2) is not a good idea, but we may want to think about (1) in the future (for qemu and lxc hook scripts as well) - instead of just looking for /etc/libvirt/hook/network, we could first look for /etc/libvirt/hook/network.${netname} and exec that instead if found (or in addition). But I think that can be deferred until later.

Actually I kind of like the option (2). I think it could make a lot of sense that the hook would be able to add an attribute to the network definition xml, e.g. <bandwidth hooked="1"> so that libvirt would know that that part has been taken care of by the hook. Of course, it might be a bad idea for libvirt to blindly accept any kind of modification, but something like what I propose does not seem eminently dangerous.

ACK if you add the <taint> element to the network status XML, and taint the domain any time it uses a tainted network.

...

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 16 ++++++++++++++++ src/conf/network_conf.h | 17 +++++++++++++++++ src/libvirt_private.syms | 3 +++ src/network/bridge_driver.c | 26 ++++++++++++++++++++++++++ 4 files changed, 62 insertions(+)

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index e59938c..aa881d8 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -72,6 +72,22 @@ VIR_ENUM_IMPL(virNetworkDNSForwardPlainNames, "yes", "no")

+VIR_ENUM_IMPL(virNetworkTaint, VIR_NETWORK_TAINT_LAST, + "hook-script"); + +bool +virNetworkObjTaint(virNetworkObjPtr obj, + enum virNetworkTaintFlags taint) +{ + unsigned int flag = (1 << taint); + + if (obj->taint & flag) + return false; + + obj->taint |= flag; + return true; +} + virNetworkObjPtr virNetworkFindByUUID(virNetworkObjListPtr nets, const unsigned char *uuid) { diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index b84762a..edcc49f 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -287,6 +287,8 @@ struct _virNetworkObj {

virBitmapPtr class_id; /* bitmap of class IDs for QoS */ unsigned long long floor_sum; /* sum of all 'floor'-s of attached NICs */ + + unsigned int taint; };

typedef struct _virNetworkObjList virNetworkObjList; @@ -296,12 +298,26 @@ struct _virNetworkObjList { virNetworkObjPtr *objs; };

+enum virNetworkTaintFlags { + VIR_NETWORK_TAINT_HOOK, /* Hook script was executed over + network. We can't guarantee + connectivity or other settings + as the script may have played + with iptables, tc, you name it. + */ + + VIR_NETWORK_TAINT_LAST +}; + static inline int virNetworkObjIsActive(const virNetworkObj *net) { return net->active; }

+bool virNetworkObjTaint(virNetworkObjPtr obj, + enum virNetworkTaintFlags taint); + virNetworkObjPtr virNetworkFindByUUID(virNetworkObjListPtr nets, const unsigned char *uuid); virNetworkObjPtr virNetworkFindByName(virNetworkObjListPtr nets, @@ -452,4 +468,5 @@ virNetworkDefUpdateSection(virNetworkDefPtr def, const char *xml, unsigned int flags); /* virNetworkUpdateFlags */

+VIR_ENUM_DECL(virNetworkTaint) #endif /* __NETWORK_CONF_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c5a7637..0759d73 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -525,6 +525,7 @@ virNetworkObjListFree; virNetworkObjLock; virNetworkObjReplacePersistentDef; virNetworkObjSetDefTransient; +virNetworkObjTaint; virNetworkObjUnlock; virNetworkObjUnsetDefTransient; virNetworkObjUpdate; @@ -533,6 +534,8 @@ virNetworkSaveConfig; virNetworkSaveStatus; virNetworkSetBridgeMacAddr; virNetworkSetBridgeName; +virNetworkTaintTypeFromString; +virNetworkTaintTypeToString; virPortGroupFindByName;

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 1ba2b2d..f2aef48 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -112,6 +112,9 @@ static int networkPlugBandwidth(virNetworkObjPtr net, static int networkUnplugBandwidth(virNetworkObjPtr net, virDomainNetDefPtr iface);

+static void networkNetworkObjTaint(virNetworkObjPtr net, + enum virNetworkTaintFlags taint); + static virNetworkDriverStatePtr driverState = NULL;

static virNetworkObjPtr @@ -2024,6 +2027,8 @@ networkStartNetwork(virNetworkDriverStatePtr driver, */ if (hookret < 0) goto cleanup; + + networkNetworkObjTaint(network, VIR_NETWORK_TAINT_HOOK); }

switch (network->def->forward.type) { @@ -2067,6 +2072,8 @@ networkStartNetwork(virNetworkDriverStatePtr driver, */ if (hookret < 0) goto cleanup; + + networkNetworkObjTaint(network, VIR_NETWORK_TAINT_HOOK); }

network->active = 1; @@ -3649,6 +3656,8 @@ validate: */ if (hookret < 0) goto error; + + networkNetworkObjTaint(network, VIR_NETWORK_TAINT_HOOK); }

if (dev) { @@ -4023,6 +4032,8 @@ success: VIR_HOOK_NETWORK_OP_IFACE_UNPLUGGED, VIR_HOOK_SUBOP_BEGIN, NULL, xml, NULL); VIR_FREE(xml); + + networkNetworkObjTaint(network, VIR_NETWORK_TAINT_HOOK); }

VIR_DEBUG("Releasing network %s, %d connections", @@ -4359,3 +4370,18 @@ networkUnplugBandwidth(virNetworkObjPtr net, cleanup: return ret; } + +static void +networkNetworkObjTaint(virNetworkObjPtr net, + enum virNetworkTaintFlags taint) +{ + if (virNetworkObjTaint(net, taint)) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(net->def->uuid, uuidstr); + + VIR_WARN("Network name='%s' uuid=%s is tainted: %s", + net->def->name, + uuidstr, + virNetworkTaintTypeToString(taint)); + } +}

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