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(a)redhat.com&gt; --- 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 0b43a67..53c2274 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1995,23 +1995,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; + goto cleanup; } + VIR_DEBUG("Beginning network startup process"); + + VIR_DEBUG("Setting current network def as transient"); if (virNetworkObjSetDefTransient(network, true) < 0) - return -1; + goto cleanup; 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: @@ -2019,28 +2025,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); -- 1.8.5.2

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(a)redhat.com&gt; --- docs/hooks.html.in | 43 +++++++++++++++++++++++++++++-------------- src/network/bridge_driver.c | 29 +++++++++++++++++++++++++++++ src/util/virhook.c | 10 +++++++++- src/util/virhook.h | 8 ++++++++ 4 files changed, 75 insertions(+), 15 deletions(-) diff --git a/docs/hooks.html.in b/docs/hooks.html.in index f0f692b..b950031 100644 --- a/docs/hooks.html.in +++ b/docs/hooks.html.in @@ -16,6 +16,7 @@ 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> + <li>A network is started or stopped<br/><br/></li> </ul> <h2><a name="location">Script location</a></h2> @@ -44,6 +45,8 @@ 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</li> </ul> <br/> @@ -62,10 +65,11 @@ <h2><a name="arguments">Script arguments</a></h2> <p>The hook scripts are called with specific command line arguments, depending upon the script, and the operation being performed.</p> - <p>The guest hook scripts, qemu and lxc, are also given the <b>full</b> - 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 guest hook scripts, qemu and lxc, or network hook script are + also given the <b>full</b> XML description for the domain on their + stdin. This includes items such as the UUID of the domain or network, + domain storage information, etc. and is intended to provide all the + libvirt information the script needs.</p> <p>The command line arguments take this approach:</p> <ol> @@ -181,23 +185,34 @@ <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>, when a network is started, + this script is called as:<br/> + <pre>/etc/libvirt/hooks/network network_name start - start</pre></li> + <li>When a network is shut down, this script is called as:<br/> + <pre>/etc/libvirt/hooks/network network_name shutdown - shutdown</pre></li> + </ul> + <br/> <h2><a name="execution">Script execution</a></h2> <ul> - <li>The "start" operation for the guest hook scripts, qemu and lxc, + <li>The "start" operation for the guest and network hook scripts, 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> + guest or network 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 guest or network has stopped. If the hook + script indicates failure in its return, the shut down of the guest + or network 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 daemon is started.<br/><br/></li> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 53c2274..2bca5bc 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 @@ -2011,6 +2012,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); + 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: @@ -2090,6 +2108,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; diff --git a/src/util/virhook.c b/src/util/virhook.c index 159efdb..5206810 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,10 @@ VIR_ENUM_IMPL(virHookLxcOp, VIR_HOOK_LXC_OP_LAST, "started", "reconnect") +VIR_ENUM_IMPL(virHookNetworkOp, VIR_HOOK_NETWORK_OP_LAST, + "start", + "stopped") + static int virHooksFound = -1; /** @@ -246,6 +252,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..05ed3b5 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,13 @@ enum virHookLxcOpType { VIR_HOOK_LXC_OP_LAST, }; +enum virHookNetworkOpType { + VIR_HOOK_NETWORK_OP_START, /* network is about to start */ + VIR_HOOK_NETWORK_OP_STOPPED, /* network has stopped */ + + VIR_HOOK_NETWORK_OP_LAST, +}; + int virHookInitialize(void); int virHookPresent(int driver); -- 1.8.5.2

On Mon, Feb 03, 2014 at 12:36:32PM +0100, Michal Privoznik wrote: We use 'tainting' as a way to identify anything which could alter the operation/behaviour of the VM in a way that isn't obvious from the XML config. As such I'd say that use of hooks should cause tainting, and likewise we should have a tainting concept for networks too. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 03.02.2014 15:21, Laine Stump wrote: What a clever idea! If users are to use hooks for their own QoS implementation they certainly need this. Then, I'm for network + whole domain XML - it doesn't hurt to have more information, it can hurt if we now introduce abbreviated XML => sooner or later somebody will come and say the domain XML is shortened too much. Michal