[libvirt] [PATCH] [0/6] implement synchronous hook scripts
As discussed previously this implement hook scripts which may be called from libvirtd at specific times. The script are supposed to be small, execute fast as it's synchronous, their exit value is 0 otherwise it's considered a failure (which may be ignored but in general will stops the ongoing operation). Scripts are stored under /etc/libvirt/hook/ (or rather $SYSCONF_DIR/libvirt/hook/) if missing no script invocation will ever be done. There is one script per 'driver' there is currently hooks for the daemon, qemu and lxc, so right now only /etc/libvirt/hook/daemon /etc/libvirt/hook/qemu /etc/libvirt/hook/lxc are potentially used. The implemented set of operations is rather simple currently daemon start, reload, and exit, and for domain operations, domain startup and exit. This can be extended to more drivers in the future of more fine-grained modularity for the scripts. The current API to the script israther simple it get passed arguments about the object name, the operation name, suboperation, and an extra string. Right now for the daemon the script would get the fallowing calls: /etc/libvirt/hooks/daemon - start - start /etc/libvirt/hooks/daemon - reload begin SIGHUP /etc/libvirt/hooks/daemon - shutdown - shutdown and for qemu (or lxc): /etc/libvirt/hooks/qemu RHEL-5.4-64 start begin - /etc/libvirt/hooks/qemu RHEL-5.4-64 stopped end - with the XML configuration of the domain passed on the script stdin if needed. In case of script failure at domain startup, this is raised as an normal error, e.g.: [root@paphio tmp]# virsh start RHEL-5.4-64 error: Failed to start domain RHEL-5.4-64 error: Hook script execution failed: Hook script /etc/libvirt/hooks/qemu qemu failed with error code 256:forbidden startup [root@paphio tmp]# Some hooks could certainly be added at different place in domain lifetime operations, for example we plan to add some for migration, but one thing to remember is that those synchronous scripts can have a serious impact on execution, so to some extend it's better to use the asynchronous async event mechanism when possible. Among the things left to improve and check, there is the behaviour when the daemon is started by the user for example for qemu:///user and double check some of the script interaction with security, SELinux in particular. But the basics are there, and I would hope to have this in the next release, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Export virPipeReadUntilEOF internally used to read the data from virExec stdout/err file descriptors * src/util/util.c src/util/util.h: not static anymore and export it * src/libvirt_private.syms: allow access internally diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f1ad6db..d9aff54 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -574,6 +574,7 @@ virFilePid; virFileReadPid; virFileLinkPointsTo; virParseNumber; +virPipeReadUntilEOF; virAsprintf; virRun; virSkipSpaces; diff --git a/src/util/util.c b/src/util/util.c index 1188e5f..62dc5f1 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -786,7 +786,7 @@ int virExecDaemonize(const char *const*argv, return ret; } -static int +int virPipeReadUntilEOF(int outfd, int errfd, char **outbuf, char **errbuf) { diff --git a/src/util/util.h b/src/util/util.h index e69eb5c..24dfbfc 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -81,6 +81,8 @@ int virRun(const char *const*argv, int *status) ATTRIBUTE_RETURN_CHECK; int virRunWithHook(const char *const*argv, virExecHook hook, void *data, int *status) ATTRIBUTE_RETURN_CHECK; +int virPipeReadUntilEOF(int outfd, int errfd, + char **outbuf, char **errbuf); int virFork(pid_t *pid); int virFileReadLimFD(int fd, int maxlen, char **buf) ATTRIBUTE_RETURN_CHECK; -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 03/26/2010 09:41 AM, Daniel Veillard wrote:
Export virPipeReadUntilEOF internally
used to read the data from virExec stdout/err file descriptors
* src/util/util.c src/util/util.h: not static anymore and export it * src/libvirt_private.syms: allow access internally
ACK. Minor nit, though:
virParseNumber; +virPipeReadUntilEOF; virAsprintf; virRun;
Is this list intended to be alphabetical? If so, why is virAsprintf where it is? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Fri, Mar 26, 2010 at 10:09:53AM -0600, Eric Blake wrote:
On 03/26/2010 09:41 AM, Daniel Veillard wrote:
Export virPipeReadUntilEOF internally
used to read the data from virExec stdout/err file descriptors
* src/util/util.c src/util/util.h: not static anymore and export it * src/libvirt_private.syms: allow access internally
ACK. Minor nit, though:
virParseNumber; +virPipeReadUntilEOF; virAsprintf; virRun;
Is this list intended to be alphabetical? If so, why is virAsprintf where it is?
It is psuedo-alphabetical, by which i mean its alphabetical, except where it isn't. It is nice to keep it alpha sorted, but we've not actively enforced that thus far....patches welcome Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Add an error module and message for the hooks subsystem * include/libvirt/virterror.h: add VIR_FROM_HOOK and VIR_ERR_HOOK_SCRIPT_FAILED * src/util/virterror.c: associated strings diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h index d298447..b35ea6c 100644 --- a/include/libvirt/virterror.h +++ b/include/libvirt/virterror.h @@ -69,7 +69,8 @@ typedef enum { VIR_FROM_PHYP, /* Error from IBM power hypervisor */ VIR_FROM_SECRET, /* Error from secret storage */ VIR_FROM_CPU, /* Error from CPU driver */ - VIR_FROM_XENAPI /* Error from XenAPI */ + VIR_FROM_XENAPI, /* Error from XenAPI */ + VIR_FROM_HOOK /* Error from Synchronous hooks */ } virErrorDomain; @@ -176,6 +177,7 @@ typedef enum { VIR_ERR_OPERATION_TIMEOUT, /* timeout occurred during operation */ VIR_ERR_MIGRATE_PERSIST_FAILED, /* a migration worked, but making the VM persist on the dest host failed */ + VIR_ERR_HOOK_SCRIPT_FAILED, /* a synchronous hook script failed */ } virErrorNumber; /** diff --git a/src/util/virterror.c b/src/util/virterror.c index 0e8bdb3..50ced0f 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -178,6 +178,9 @@ static const char *virErrorDomainName(virErrorDomain domain) { case VIR_FROM_CPU: dom = "CPU "; break; + case VIR_FROM_HOOK: + dom = "Sync Hook "; + break; } return(dom); } @@ -1117,6 +1120,11 @@ virErrorMsg(virErrorNumber error, const char *info) errmsg = _("Failed to make domain persistent after migration"); else errmsg = _("Failed to make domain persistent after migration: %s"); + case VIR_ERR_HOOK_SCRIPT_FAILED: + if (info == NULL) + errmsg = _("Hook script execution failed"); + else + errmsg = _("Hook script execution failed: %s"); break; } return (errmsg); -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 03/26/2010 09:42 AM, Daniel Veillard wrote:
@@ -1117,6 +1120,11 @@ virErrorMsg(virErrorNumber error, const char *info) errmsg = _("Failed to make domain persistent after migration"); else errmsg = _("Failed to make domain persistent after migration: %s"); + case VIR_ERR_HOOK_SCRIPT_FAILED: + if (info == NULL) + errmsg = _("Hook script execution failed"); + else + errmsg = _("Hook script execution failed: %s");
We don't have a very consistent style. A quick glance at virErrorMsg shows that maybe half of the messages start lower-case, and the other half start upper-case. GNU style recommends that error messages start with lower-case letters (unless the first word is an acronym, such as in the error _("GET operation failed") for VIR_ERR_GET_FAILED), and although this is not a GNU project, there is something to be said for consistency. Is it worth a separate patch to make error message consistently start with lower-case? maint.mk even has a syntax-check rule that we can use to help in this regards (sc_error_message_uppercase), although we are not currently using it. At any rate, capitalization can be a separate cleanup patch, so ACK to this one. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Fri, Mar 26, 2010 at 10:16:37AM -0600, Eric Blake wrote:
On 03/26/2010 09:42 AM, Daniel Veillard wrote:
@@ -1117,6 +1120,11 @@ virErrorMsg(virErrorNumber error, const char *info) errmsg = _("Failed to make domain persistent after migration"); else errmsg = _("Failed to make domain persistent after migration: %s"); + case VIR_ERR_HOOK_SCRIPT_FAILED: + if (info == NULL) + errmsg = _("Hook script execution failed"); + else + errmsg = _("Hook script execution failed: %s");
We don't have a very consistent style. A quick glance at virErrorMsg shows that maybe half of the messages start lower-case, and the other half start upper-case. GNU style recommends that error messages start with lower-case letters (unless the first word is an acronym, such as in the error _("GET operation failed") for VIR_ERR_GET_FAILED), and although this is not a GNU project, there is something to be said for consistency.
Is it worth a separate patch to make error message consistently start with lower-case? maint.mk even has a syntax-check rule that we can use to help in this regards (sc_error_message_uppercase), although we are not currently using it.
At any rate, capitalization can be a separate cleanup patch, so ACK to this one.
and in general that duplication of _("") strings is just nasty, we need to fix it, this requires translators to type everything twice it's nasty, awful, and need to be fixed. I though about this again when adding the entry, but I didn't want to mix issues. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Fri, Mar 26, 2010 at 05:42:22PM +0100, Daniel Veillard wrote:
On Fri, Mar 26, 2010 at 10:16:37AM -0600, Eric Blake wrote:
On 03/26/2010 09:42 AM, Daniel Veillard wrote:
@@ -1117,6 +1120,11 @@ virErrorMsg(virErrorNumber error, const char *info) errmsg = _("Failed to make domain persistent after migration"); else errmsg = _("Failed to make domain persistent after migration: %s"); + case VIR_ERR_HOOK_SCRIPT_FAILED: + if (info == NULL) + errmsg = _("Hook script execution failed"); + else + errmsg = _("Hook script execution failed: %s");
We don't have a very consistent style. A quick glance at virErrorMsg shows that maybe half of the messages start lower-case, and the other half start upper-case. GNU style recommends that error messages start with lower-case letters (unless the first word is an acronym, such as in the error _("GET operation failed") for VIR_ERR_GET_FAILED), and although this is not a GNU project, there is something to be said for consistency.
Is it worth a separate patch to make error message consistently start with lower-case? maint.mk even has a syntax-check rule that we can use to help in this regards (sc_error_message_uppercase), although we are not currently using it.
At any rate, capitalization can be a separate cleanup patch, so ACK to this one.
and in general that duplication of _("") strings is just nasty, we need to fix it, this requires translators to type everything twice it's nasty, awful, and need to be fixed. I though about this again when adding the entry, but I didn't want to mix issues.
I did a proof of concept converting both of the switch() blocks into a VIR_ENUM style lookup a while ago, but its bit-rotted. It was much nicer though. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Add hook utilities This exports 3 basic routines: - virHookInitialize() initializing the hook support by looking for scripts availability - virHookPresent() used to test if there is a hook for a given driver - virHookCall() which actually calls a synchronous script hook with the needed parameters Note that this doesn't expose any public API except for the locations and arguments passed to the scripts * src/Makefile.am: add the 2 new files * src/util/hooks.h src/util/hooks.c: implements the 3 functions * src/libvirt_private.syms: export the 3 symbols internally diff --git a/src/Makefile.am b/src/Makefile.am index 5f6b325..8f1e549 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -54,6 +54,7 @@ UTIL_SOURCES = \ util/cgroup.c util/cgroup.h \ util/event.c util/event.h \ util/hash.c util/hash.h \ + util/hooks.c util/hooks.h \ util/iptables.c util/iptables.h \ util/ebtables.c util/ebtables.h \ util/json.c util/json.h \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d9aff54..4e4f126 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -242,6 +242,12 @@ virHashSearch; virHashSize; +# hooks.h +virHookCall; +virHookInitialize; +virHookPresent; + + # interface_conf.h virInterfaceDefFormat; virInterfaceDefParseFile; diff --git a/src/util/hooks.c b/src/util/hooks.c new file mode 100644 index 0000000..3c027d6 --- /dev/null +++ b/src/util/hooks.c @@ -0,0 +1,439 @@ +/* + * hooks.c: implementation of the synchronous hooks support + * + * Copyright (C) 2010 Red Hat, Inc. + * Copyright (C) 2010 Daniel Veillard + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Author: Daniel Veillard <veillard@redhat.com> + */ + +#include <config.h> + +#include <sys/types.h> +#include <sys/wait.h> +#include <sys/stat.h> +#include <unistd.h> +#include <stdlib.h> +#include <stdio.h> + +#include "virterror_internal.h" +#include "hooks.h" +#include "util.h" +#include "conf/domain_conf.h" +#include "logging.h" +#include "memory.h" + +#define VIR_FROM_THIS VIR_FROM_HOOK + +#define virHookReportError(code, ...) \ + virReportErrorHelper(NULL, VIR_FROM_HOOK, code, __FILE__, \ + __FUNCTION__, __LINE__, __VA_ARGS__) + +#define LIBVIRT_HOOK_DIR SYSCONF_DIR "/libvirt/hooks" + +VIR_ENUM_DECL(virHookDriver) +VIR_ENUM_DECL(virHookDaemonOp) +VIR_ENUM_DECL(virHookSubop) +VIR_ENUM_DECL(virHookQemuOp) +VIR_ENUM_DECL(virHookLxcOp) + +VIR_ENUM_IMPL(virHookDriver, + VIR_HOOK_DRIVER_LAST, + "daemon", + "qemu", + "lxc") + +VIR_ENUM_IMPL(virHookDaemonOp, VIR_HOOK_DAEMON_OP_LAST, + "start", + "shutdown", + "reload") + +VIR_ENUM_IMPL(virHookSubop, VIR_HOOK_SUBOP_LAST, + "-", + "begin", + "end") + +VIR_ENUM_IMPL(virHookQemuOp, VIR_HOOK_QEMU_OP_LAST, + "start", + "stopped") + +VIR_ENUM_IMPL(virHookLxcOp, VIR_HOOK_QEMU_OP_LAST, + "start", + "stopped") + +static int virHooksFound = -1; + +/** + * virHookCheck: + * @driver: the driver name "daemon", "qemu", "lxc"... + * + * Check is there is an installed hook for the given driver, if this + * is the case register it. Then subsequent calls to virHookCall + * will call the hook if found. + * + * Returns 1 if found, 0 if not found, and -1 in case of error + */ +static int +virHookCheck(int no, const char *driver) { + char *path; + struct stat sb; + int ret; + + if (driver == NULL) { + virHookReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid hook name for #%d"), no); + return(-1); + } + + ret = virBuildPath(&path, LIBVIRT_HOOK_DIR, driver); + if ((ret < 0) || (path == NULL)) { + virHookReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to build path for %s hook"), + driver); + return(-1); + } + + if (stat(path, &sb) < 0) { + ret = 0; + VIR_DEBUG("No hook script %s", path); + } else { + if (access(path, X_OK) != 0) { + ret = 0; + VIR_WARN("Non executable hook script %s", path); + } else { + ret = 1; + VIR_DEBUG("Found hook script %s", path); + } + } + + VIR_FREE(path); + return(ret); +} + +/* + * virHookInitialize: + * + * Initialize syncronous hooks support. + * Check is there is an installed hook for all the drivers + * + * Returns the number of hooks found or -1 in case of failure + */ +int +virHookInitialize(void) { + int i, res, ret = 0; + + virHooksFound = 0; + for (i = 0;i < VIR_HOOK_DRIVER_LAST;i++) { + res = virHookCheck(i, virHookDriverTypeToString(i)); + if (res < 0) + return(-1); + + if (res == 1) { + virHooksFound |= (1 << i); + ret++; + } + } + return(ret); +} + +/** + * virHookPresent: + * @driver: the driver number (from virHookDriver enum) + * + * Check if a hook exists for the given driver, this is needed + * to avoid unecessary work if the hook is not present + * + * Returns 1 if present, 0 otherwise + */ +int +virHookPresent(int driver) { + if ((driver < VIR_HOOK_DRIVER_DAEMON) || + (driver >= VIR_HOOK_DRIVER_LAST)) + return(0); + if (virHooksFound == -1) + return(0); + + if ((virHooksFound & (1 << driver)) == 0) + return(0); + return(1); +} + +/* + * virHookCall: + * @driver: the driver number (from virHookDriver enum) + * @id: an id for the object '-' if non available for example on daemon hooks + * @op: the operation on the id e.g. VIR_HOOK_QEMU_OP_START + * @sub_op: a sub_operation, currently unused + * @extra: optional string informations + * @input: extra input given to the script on stdin + * + * Implement an Hook call, where the external script for the driver is + * called with the given informations. This is a synchronous call, we wait for + * execution completion + * + * Returns: 0 if the execution suceeded, 1 if the script was not found or + * invalid parameters, and -1 if script returned an error + */ +int +virHookCall(int driver, const char *id, int op, int sub_op, const char *extra, + const char *input) { + int ret, waitret, exitstatus, i; + char *path; + int argc = 0, arga = 0; + const char **argv = NULL; + int envc = 0, enva = 0; + const char **env = NULL; + const char *drvstr; + const char *opstr; + const char *subopstr; + pid_t pid; + int outfd = -1, errfd = -1; + int pipefd[2] = { -1, -1}; + char *outbuf = NULL; + char *errbuf = NULL; + + if ((driver < VIR_HOOK_DRIVER_DAEMON) || + (driver >= VIR_HOOK_DRIVER_LAST)) + return(1); + + /* + * We cache the availability of the script to minimize impact at + * runtime if no script is defined, this is being reset on SIGUP + */ + if ((virHooksFound == -1) || + ((driver == VIR_HOOK_DRIVER_DAEMON) && + (op == VIR_HOOK_DAEMON_OP_RELOAD))) + virHookInitialize(); + + if ((virHooksFound & (1 << driver)) == 0) + return(1); + + drvstr = virHookDriverTypeToString(driver); + + opstr = NULL; + switch (driver) { + case VIR_HOOK_DRIVER_DAEMON: + opstr = virHookDaemonOpTypeToString(op); + break; + case VIR_HOOK_DRIVER_QEMU: + opstr = virHookQemuOpTypeToString(op); + break; + case VIR_HOOK_DRIVER_LXC: + opstr = virHookLxcOpTypeToString(op); + break; + } + if (opstr == NULL) { + virHookReportError(VIR_ERR_INTERNAL_ERROR, + _("Hook for %s, failed to find operation #%d"), + drvstr, op); + return(1); + } + subopstr = virHookSubopTypeToString(sub_op); + if (subopstr == NULL) + subopstr = "-"; + if (extra == NULL) + extra = "-"; + + ret = virBuildPath(&path, LIBVIRT_HOOK_DIR, drvstr); + if ((ret < 0) || (path == NULL)) { + virHookReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to build path for %s hook"), + drvstr); + return(-1); + } + + /* + * Convenience macros borrowed from qemudBuildCommandLine() + */ +#define ADD_ARG_SPACE \ + do { \ + if (argc == arga) { \ + arga += 10; \ + if (VIR_REALLOC_N(argv, arga) < 0) \ + goto no_memory; \ + } \ + } while (0) + +#define ADD_ARG(thisarg) \ + do { \ + ADD_ARG_SPACE; \ + argv[argc++] = thisarg; \ + } while (0) + +#define ADD_ARG_LIT(thisarg) \ + do { \ + ADD_ARG_SPACE; \ + if ((argv[argc++] = strdup(thisarg)) == NULL) \ + goto no_memory; \ + } while (0) + +#define ADD_ENV_SPACE \ + do { \ + if (envc == enva) { \ + enva += 10; \ + if (VIR_REALLOC_N(env, enva) < 0) \ + goto no_memory; \ + } \ + } while (0) + +#define ADD_ENV(thisarg) \ + do { \ + ADD_ENV_SPACE; \ + env[envc++] = thisarg; \ + } while (0) + +#define ADD_ENV_LIT(thisarg) \ + do { \ + ADD_ENV_SPACE; \ + if ((env[envc++] = strdup(thisarg)) == NULL) \ + goto no_memory; \ + } while (0) + +#define ADD_ENV_PAIR(envname, val) \ + do { \ + char *envval; \ + ADD_ENV_SPACE; \ + if (virAsprintf(&envval, "%s=%s", envname, val) < 0) \ + goto no_memory; \ + env[envc++] = envval; \ + } while (0) + +#define ADD_ENV_COPY(envname) \ + do { \ + char *val = getenv(envname); \ + if (val != NULL) { \ + ADD_ENV_PAIR(envname, val); \ + } \ + } while (0) + + ADD_ENV_LIT("LC_ALL=C"); + + ADD_ENV_COPY("LD_PRELOAD"); + ADD_ENV_COPY("LD_LIBRARY_PATH"); + ADD_ENV_COPY("PATH"); + ADD_ENV_COPY("HOME"); + ADD_ENV_COPY("USER"); + ADD_ENV_COPY("LOGNAME"); + ADD_ENV_COPY("TMPDIR"); + ADD_ENV(NULL); + + ADD_ARG_LIT(path); + ADD_ARG_LIT(id); + ADD_ARG_LIT(opstr); + ADD_ARG_LIT(subopstr); + + ADD_ARG_LIT(extra); + ADD_ARG(NULL); + + /* pass any optional input on the script stdin */ + if (input != NULL) { + if (pipe(pipefd) < -1) { + virReportSystemError(errno, "%s", + _("unable to create pipe for hook input")); + ret = 1; + goto cleanup; + } + if (safewrite(pipefd[1], input, strlen(input)) < 0) { + virReportSystemError(errno, "%s", + _("unable to write to pipe for hook input")); + ret = 1; + goto cleanup; + } + ret = virExec(argv, env, NULL, &pid, pipefd[0], &outfd, &errfd, + VIR_EXEC_NONE | VIR_EXEC_NONBLOCK); + close(pipefd[1]); + pipefd[1] = -1; + } else { + ret = virExec(argv, env, NULL, &pid, -1, &outfd, &errfd, + VIR_EXEC_NONE | VIR_EXEC_NONBLOCK); + } + if (ret < 0) { + virHookReportError(VIR_ERR_HOOK_SCRIPT_FAILED, + _("Failed to execute %s hook script"), + path); + ret = 1; + goto cleanup; + } + + /* + * we are interested in the error log if any and make sure the + * script doesn't block on stdout/stderr descriptors being full + * stdout can be useful for debug too. + */ + if (virPipeReadUntilEOF(outfd, errfd, &outbuf, &errbuf) < 0) { + virReportSystemError(errno, _("cannot wait for '%s'"), path); + while (waitpid(pid, &exitstatus, 0) == -1 && errno == EINTR) + ; + ret = 1; + goto cleanup; + } + + if (outbuf) + VIR_DEBUG("Command stdout: %s", outbuf); + if (errbuf) + VIR_DEBUG("Command stderr: %s", errbuf); + + while ((waitret = waitpid(pid, &exitstatus, 0) == -1) && + (errno == EINTR)); + if (waitret == -1) { + virReportSystemError(errno, _("Failed to wait for '%s'"), path); + ret = 1; + goto cleanup; + } + if (exitstatus != 0) { + virHookReportError(VIR_ERR_HOOK_SCRIPT_FAILED, + _("Hook script %s %s failed with error code %d:%s"), + path, drvstr, exitstatus, errbuf); + ret = -1; + } + +cleanup: + if (pipefd[0] > 0) + close(pipefd[0]); + if (pipefd[1] > 0) + close(pipefd[1]); + if (argv) { + for (i = 0 ; i < argc ; i++) + VIR_FREE((argv)[i]); + VIR_FREE(argv); + } + if (env) { + for (i = 0 ; i < envc ; i++) + VIR_FREE((env)[i]); + VIR_FREE(env); + } + VIR_FREE(outbuf); + VIR_FREE(errbuf); + VIR_FREE(path); + + return(ret); + +no_memory: + virReportOOMError(); + + goto cleanup; + +#undef ADD_ARG +#undef ADD_ARG_LIT +#undef ADD_ARG_SPACE +#undef ADD_USBDISK +#undef ADD_ENV +#undef ADD_ENV_COPY +#undef ADD_ENV_LIT +#undef ADD_ENV_SPACE +} + diff --git a/src/util/hooks.h b/src/util/hooks.h new file mode 100644 index 0000000..28fb17e --- /dev/null +++ b/src/util/hooks.h @@ -0,0 +1,77 @@ +/* + * hook.h: internal entry points needed for synchronous hooks support + * + * Copyright (C) 2010 Red Hat, Inc. + * Copyright (C) 2010 Daniel Veillard + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Author: Daniel Veillard <veillard@redhat.com> + */ + +#ifndef __VIR_HOOKS_H__ +# define __VIR_HOOKS_H__ + +# include "internal.h" +# include "util.h" + +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_LAST, +}; + +enum virHookDaemonOpType { + VIR_HOOK_DAEMON_OP_START, /* daemon is about to start */ + VIR_HOOK_DAEMON_OP_SHUTDOWN, /* daemon is about to shutdown */ + VIR_HOOK_DAEMON_OP_RELOAD, /* driver reload with SIGHUP */ + + VIR_HOOK_DAEMON_OP_LAST, +}; + +enum virHookSubopType { + VIR_HOOK_SUBOP_NONE, /* no sub-operation */ + VIR_HOOK_SUBOP_BEGIN, /* beginning of the operation */ + VIR_HOOK_SUBOP_END, /* end of the operation */ + + VIR_HOOK_SUBOP_LAST, +}; + +enum virHookQemuOpType { + VIR_HOOK_QEMU_OP_START, /* domain is about to start */ + VIR_HOOK_QEMU_OP_STOPPED, /* domain has stopped */ + + VIR_HOOK_QEMU_OP_LAST, +}; + +enum virHookLxcOpType { + VIR_HOOK_LXC_OP_START, /* domain is about to start */ + VIR_HOOK_LXC_OP_STOPPED, /* domain has stopped */ + + VIR_HOOK_LXC_OP_LAST, +}; + +int virHookInitialize(void); + +int virHookPresent(int driver); + +int virHookCall(int driver, const char *id, int op, int sub_op, + const char *extra, const char *input); + +#endif /* __VIR_HOOKS_H__ */ + + -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 03/26/2010 09:43 AM, Daniel Veillard wrote:
+ if (stat(path, &sb) < 0) { + ret = 0; + VIR_DEBUG("No hook script %s", path); + } else { + if (access(path, X_OK) != 0) {
Should we also check for !S_ISDIR(&sb.st_mode), so that we explicitly reject directories here, rather than failing later when trying to execute them? Or go one step further and require regular files, with the stricter check for S_ISREG(&sb.st_mode)? (Note: symlinks to regular files would still be okay, given that you used stat().)
+ * virHookInitialize: + * + * Initialize syncronous hooks support.
s/syncronous/synchronous/
+/** + * virHookPresent: + * @driver: the driver number (from virHookDriver enum) + * + * Check if a hook exists for the given driver, this is needed + * to avoid unecessary work if the hook is not present
s/unecessary/unnecessary/
+/* + * virHookCall: + * @driver: the driver number (from virHookDriver enum) + * @id: an id for the object '-' if non available for example on daemon hooks + * @op: the operation on the id e.g. VIR_HOOK_QEMU_OP_START + * @sub_op: a sub_operation, currently unused + * @extra: optional string informations
s/informations/information/ (multiple times)
+ * @input: extra input given to the script on stdin + * + * Implement an Hook call, where the external script for the driver is
s/an Hook/a Hook/ English is funny on 'a' vs. 'an' before a word starting in 'h'; sometimes you just have to use a native speaker to get it right ;)
+ * called with the given informations. This is a synchronous call, we wait for + * execution completion + * + * Returns: 0 if the execution suceeded, 1 if the script was not found or
s/suceeded/succeeded/
+ * invalid parameters, and -1 if script returned an error + */ +int +virHookCall(int driver, const char *id, int op, int sub_op, const char *extra, + const char *input) { + int ret, waitret, exitstatus, i; + char *path; + int argc = 0, arga = 0; + const char **argv = NULL; + int envc = 0, enva = 0; + const char **env = NULL; + const char *drvstr; + const char *opstr; + const char *subopstr; + pid_t pid; + int outfd = -1, errfd = -1; + int pipefd[2] = { -1, -1}; + char *outbuf = NULL; + char *errbuf = NULL; + + if ((driver < VIR_HOOK_DRIVER_DAEMON) || + (driver >= VIR_HOOK_DRIVER_LAST)) + return(1); + + /* + * We cache the availability of the script to minimize impact at + * runtime if no script is defined, this is being reset on SIGUP
s/SIGUP/SIGHUP/
+ */ + if ((virHooksFound == -1) || + ((driver == VIR_HOOK_DRIVER_DAEMON) && + (op == VIR_HOOK_DAEMON_OP_RELOAD))) + virHookInitialize(); + ... + /* + * Convenience macros borrowed from qemudBuildCommandLine() + */
Duplicating the definition of all these helpers is a bit of a long distraction in the middle of this function; is it worth a helper file, where we could #include "command_line_builder.h" to get all these helpers defined, and to reduce the duplication from qemudBuildCommandLine by having the macros in one place?
+ ADD_ENV_LIT("LC_ALL=C"); + + ADD_ENV_COPY("LD_PRELOAD"); + ADD_ENV_COPY("LD_LIBRARY_PATH"); + ADD_ENV_COPY("PATH"); + ADD_ENV_COPY("HOME"); + ADD_ENV_COPY("USER"); + ADD_ENV_COPY("LOGNAME"); + ADD_ENV_COPY("TMPDIR"); + ADD_ENV(NULL);
Not new to your patch, but POSIX 2008 recommends that we should really be checking confstr(_CS_V7_ENV), when that exists, for any other environment variables that we might also need to copy to keep the child process in an environment equally as conforming as the parent (for example, whether POSIXLY_CORRECT needs to be propagated). Probably not worth worrying about, though, unless someone identifies an actual bug.
+ + ADD_ARG_LIT(path); + ADD_ARG_LIT(id); + ADD_ARG_LIT(opstr); + ADD_ARG_LIT(subopstr); + + ADD_ARG_LIT(extra); + ADD_ARG(NULL); + + /* pass any optional input on the script stdin */ + if (input != NULL) { + if (pipe(pipefd) < -1) { + virReportSystemError(errno, "%s", + _("unable to create pipe for hook input")); + ret = 1; + goto cleanup; + } + if (safewrite(pipefd[1], input, strlen(input)) < 0) { + virReportSystemError(errno, "%s", + _("unable to write to pipe for hook input")); + ret = 1; + goto cleanup; + } + ret = virExec(argv, env, NULL, &pid, pipefd[0], &outfd, &errfd, + VIR_EXEC_NONE | VIR_EXEC_NONBLOCK); + close(pipefd[1]);
Should we be checking for close() failures, and reporting a system error? I only saw minor problems as pointed out above; the overall patch looks technically sound. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Fri, Mar 26, 2010 at 10:53:01AM -0600, Eric Blake wrote:
On 03/26/2010 09:43 AM, Daniel Veillard wrote:
+ */ + if ((virHooksFound == -1) || + ((driver == VIR_HOOK_DRIVER_DAEMON) && + (op == VIR_HOOK_DAEMON_OP_RELOAD))) + virHookInitialize(); + ... + /* + * Convenience macros borrowed from qemudBuildCommandLine() + */
Duplicating the definition of all these helpers is a bit of a long distraction in the middle of this function; is it worth a helper file, where we could #include "command_line_builder.h" to get all these helpers defined, and to reduce the duplication from qemudBuildCommandLine by having the macros in one place?
I'd like to kill off all these macros completely, and instead introduce a formal API. In a similar style to virBuffer, have a statically initialized struct, APIs to add args & env variables, and a final API to check whether any OOM errors occurred. Then a thin wrapper for virExec/virRun to actually execute it typedef struct { int hasError; char **argv; char **env; } virCommandInfo; virCommandInfo cmd = VIR_COMMAND_INFO_INITIALIZER; virCommandInfoAddArg(info, "/usr/bin/kvm"); virCommandInfoCopyEnv(info, "PATH"); virCommandInfoSetEnv(info, "LANG", "C"); virCommandInfoAddArg(info, "--foo"); virCommandInfoAddArg(info, "--bar"); if (virCommandInfoHasError(info)) virReportOOMError(); virRunCommand(info); Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On 03/26/2010 11:09 AM, Daniel P. Berrange wrote:
Duplicating the definition of all these helpers is a bit of a long distraction in the middle of this function; is it worth a helper file, where we could #include "command_line_builder.h" to get all these helpers defined, and to reduce the duplication from qemudBuildCommandLine by having the macros in one place?
I'd like to kill off all these macros completely, and instead introduce a formal API. In a similar style to virBuffer, have a statically initialized struct, APIs to add args & env variables, and a final API to check whether any OOM errors occurred. Then a thin wrapper for virExec/virRun to actually execute it
typedef struct { int hasError; char **argv; char **env; } virCommandInfo;
virCommandInfo cmd = VIR_COMMAND_INFO_INITIALIZER;
virCommandInfoAddArg(info, "/usr/bin/kvm"); virCommandInfoCopyEnv(info, "PATH"); virCommandInfoSetEnv(info, "LANG", "C"); virCommandInfoAddArg(info, "--foo"); virCommandInfoAddArg(info, "--bar");
if (virCommandInfoHasError(info)) virReportOOMError();
virRunCommand(info);
Even nicer. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Fri, Mar 26, 2010 at 10:53:01AM -0600, Eric Blake wrote:
On 03/26/2010 09:43 AM, Daniel Veillard wrote:
+ if (stat(path, &sb) < 0) { + ret = 0; + VIR_DEBUG("No hook script %s", path); + } else { + if (access(path, X_OK) != 0) {
Should we also check for !S_ISDIR(&sb.st_mode), so that we explicitly reject directories here, rather than failing later when trying to execute them? Or go one step further and require regular files, with the stricter check for S_ISREG(&sb.st_mode)? (Note: symlinks to regular files would still be okay, given that you used stat().)
Right, I'm adding this
+ * virHookInitialize: + * + * Initialize syncronous hooks support.
s/syncronous/synchronous/
+/** + * virHookPresent: + * @driver: the driver number (from virHookDriver enum) + * + * Check if a hook exists for the given driver, this is needed + * to avoid unecessary work if the hook is not present
s/unecessary/unnecessary/
+/* + * virHookCall: + * @driver: the driver number (from virHookDriver enum) + * @id: an id for the object '-' if non available for example on daemon hooks + * @op: the operation on the id e.g. VIR_HOOK_QEMU_OP_START + * @sub_op: a sub_operation, currently unused + * @extra: optional string informations
s/informations/information/ (multiple times)
+ * @input: extra input given to the script on stdin + * + * Implement an Hook call, where the external script for the driver is
s/an Hook/a Hook/ English is funny on 'a' vs. 'an' before a word starting in 'h'; sometimes you just have to use a native speaker to get it right ;)
fixed the typos, thanks for raising them, btw I would normally spot 'an hook' as a problem, but I guess I didn't reread thoise comments, thanks for spotting those !
+ /* + * Convenience macros borrowed from qemudBuildCommandLine() + */
Duplicating the definition of all these helpers is a bit of a long distraction in the middle of this function; is it worth a helper file, where we could #include "command_line_builder.h" to get all these helpers defined, and to reduce the duplication from qemudBuildCommandLine by having the macros in one place?
We can clean this up as Dan suggested, but that should be done separately, there is a few place in the code where we do this
+ ret = virExec(argv, env, NULL, &pid, pipefd[0], &outfd, &errfd, + VIR_EXEC_NONE | VIR_EXEC_NONBLOCK); + close(pipefd[1]);
Should we be checking for close() failures, and reporting a system error?
yes, but not changing behaviour, I fixed those allowed me to spot that if (pipefd[0] > 0) test on function exit should really be >= as 0 is a valid fd, so fixed those too
I only saw minor problems as pointed out above; the overall patch looks technically sound.
thanks, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Mon, Mar 29, 2010 at 04:47:54PM +0200, Daniel Veillard wrote:
On Fri, Mar 26, 2010 at 10:53:01AM -0600, Eric Blake wrote:
On 03/26/2010 09:43 AM, Daniel Veillard wrote:
+ if (stat(path, &sb) < 0) { + ret = 0; + VIR_DEBUG("No hook script %s", path); + } else { + if (access(path, X_OK) != 0) {
Should we also check for !S_ISDIR(&sb.st_mode), so that we explicitly reject directories here, rather than failing later when trying to execute them? Or go one step further and require regular files, with the stricter check for S_ISREG(&sb.st_mode)? (Note: symlinks to regular files would still be okay, given that you used stat().)
Right, I'm adding this
+ * virHookInitialize: + * + * Initialize syncronous hooks support.
s/syncronous/synchronous/
+/** + * virHookPresent: + * @driver: the driver number (from virHookDriver enum) + * + * Check if a hook exists for the given driver, this is needed + * to avoid unecessary work if the hook is not present
s/unecessary/unnecessary/
+/* + * virHookCall: + * @driver: the driver number (from virHookDriver enum) + * @id: an id for the object '-' if non available for example on daemon hooks + * @op: the operation on the id e.g. VIR_HOOK_QEMU_OP_START + * @sub_op: a sub_operation, currently unused + * @extra: optional string informations
s/informations/information/ (multiple times)
+ * @input: extra input given to the script on stdin + * + * Implement an Hook call, where the external script for the driver is
s/an Hook/a Hook/ English is funny on 'a' vs. 'an' before a word starting in 'h'; sometimes you just have to use a native speaker to get it right ;)
fixed the typos, thanks for raising them, btw I would normally spot 'an hook' as a problem, but I guess I didn't reread thoise comments, thanks for spotting those !
Heh... see for example "A hole" vs. "An 'ole!" --H -- ======================================================== Hugh Brock, hbrock@redhat.com, +1-215-564-3232 Deltacloud API + Portal http://deltacloud.org Libvirt virtualization library http://libvirt.org ========================================================
Add the script hook support to the libvirt daemon It supports 3 kind of probing times, at daemon startup, when the daemon reloads its drivers on SIGHUP and when the daemon exits * daemon/libvirtd.c: daemon hooks for startup, reload and exit diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index d59a2ab..150ff18 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -62,6 +62,7 @@ #include "event.h" #include "memory.h" #include "stream.h" +#include "hooks.h" #ifdef HAVE_AVAHI # include "mdns.h" #endif @@ -375,8 +376,11 @@ qemudDispatchSignalEvent(int watch ATTRIBUTE_UNUSED, switch (siginfo.si_signo) { case SIGHUP: VIR_INFO0(_("Reloading configuration on SIGHUP")); + virHookCall(VIR_HOOK_DRIVER_DAEMON, "-", + VIR_HOOK_DAEMON_OP_RELOAD, SIGHUP, "SIGHUP", NULL); if (virStateReload() < 0) VIR_WARN0(_("Error while reloading drivers")); + break; case SIGINT: @@ -3124,9 +3128,18 @@ int main(int argc, char **argv) { goto error; } + /* setup the hooks */ + virHookInitialize(); + /* Disable error func, now logging is setup */ virSetErrorFunc(NULL, virshErrorHandler); + /* + * Call the daemon startup hook + */ + virHookCall(VIR_HOOK_DRIVER_DAEMON, "-", VIR_HOOK_DAEMON_OP_START, + 0, "start", NULL); + if (qemudNetworkInit(server) < 0) { ret = VIR_DAEMON_ERR_NETWORK; goto error; @@ -3187,6 +3200,9 @@ shutdown: } pthread_join(server->eventThread, NULL); + virHookCall(VIR_HOOK_DRIVER_DAEMON, "-", VIR_HOOK_DAEMON_OP_SHUTDOWN, + 0, "shutdown", NULL); + error: if (statuswrite != -1) { if (ret != 0) { -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 03/26/2010 09:44 AM, Daniel Veillard wrote:
+ /* setup the hooks */ + virHookInitialize();
Shouldn't this check for a return of -1?
+ /* Disable error func, now logging is setup */ virSetErrorFunc(NULL, virshErrorHandler);
+ /* + * Call the daemon startup hook + */ + virHookCall(VIR_HOOK_DRIVER_DAEMON, "-", VIR_HOOK_DAEMON_OP_START, + 0, "start", NULL);
For that matter, virHookCall calls virHookInitialize under the hood, do we need to repeat that ourselves? And if we don't need to repeat it, does virHookInitialize still need to be exported? Should we check for virHookCall failure? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Fri, Mar 26, 2010 at 11:00:54AM -0600, Eric Blake wrote:
On 03/26/2010 09:44 AM, Daniel Veillard wrote:
+ /* setup the hooks */ + virHookInitialize();
Shouldn't this check for a return of -1?
Somehow, yes, as -1 should only be returned in case of serious errors, not just missing scripts or directories. So yes and I'm adding a corresponding virDaemonErr.
+ /* Disable error func, now logging is setup */ virSetErrorFunc(NULL, virshErrorHandler);
+ /* + * Call the daemon startup hook + */ + virHookCall(VIR_HOOK_DRIVER_DAEMON, "-", VIR_HOOK_DAEMON_OP_START, + 0, "start", NULL);
For that matter, virHookCall calls virHookInitialize under the hood, do we need to repeat that ourselves? And if we don't need to repeat it, does virHookInitialize still need to be exported?
I prefer to have a separate initialization routine, since we are caching the state, this routine allow to force a scan of the directory if needed. So not needed right now, but I prefer to keep it.
Should we check for virHookCall failure?
On daemon exit that would be useless since threads are already stopped, for reload I don't see that to be cancelleable. There is only at daemon start that the script could potentially return an error and prevent the daemon from starting. Weighting the pros and cons of being able to avoid the daemon versus the risk of breaking the daemon startup due to some system error, I initially decided to let the daemon start anyway. It's not a clear cut, I'm adding a comment to keep the issue open. thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Add script hook support to the QEmu driver Right now this implements only 2 basic hooks: - before the qemu process is being launched - after the qemu process is terminated the XML description of the domain is passed to the hook script stdin /etc/libvirt/hook/qemu * src/qemu/qemu_driver.c: implement synchronous script hooks for QEmu at domain startup and end diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 257f914..64222fa 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -83,6 +83,7 @@ #include "xml.h" #include "cpu/cpu.h" #include "macvtap.h" +#include "hooks.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -2858,6 +2859,22 @@ static int qemudStartVMDaemon(virConnectPtr conn, &tapfds, &ntapfds, migrateFrom) < 0) goto cleanup; + /* now that we know it is about to start call the hook if present */ + if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { + char *xml = virDomainDefFormat(vm->def, 0); + int hookret; + + hookret = virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name, + VIR_HOOK_QEMU_OP_START, VIR_HOOK_SUBOP_BEGIN, NULL, xml); + VIR_FREE(xml); + + /* + * If the script raised an error abort the launch + */ + if (hookret < 0) + goto cleanup; + } + tmp = progenv; while (*tmp) { if (safewrite(logfile, *tmp, strlen(*tmp)) < 0) @@ -3078,6 +3095,16 @@ static void qemudShutdownVMDaemon(struct qemud_driver *driver, /* shut it off for sure */ virKillProcess(vm->pid, SIGKILL); + /* now that we know it's stopped call the hook if present */ + if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { + char *xml = virDomainDefFormat(vm->def, 0); + + /* we can't stop the operation even if the script raised an error */ + virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name, + VIR_HOOK_QEMU_OP_STOPPED, VIR_HOOK_SUBOP_END, NULL, xml); + VIR_FREE(xml); + } + /* Reset Security Labels */ if (driver->securityDriver && driver->securityDriver->domainRestoreSecurityAllLabel) -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 03/26/2010 09:45 AM, Daniel Veillard wrote:
+ /* now that we know it is about to start call the hook if present */ + if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { + char *xml = virDomainDefFormat(vm->def, 0); + int hookret; + + hookret = virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name, + VIR_HOOK_QEMU_OP_START, VIR_HOOK_SUBOP_BEGIN, NULL, xml); + VIR_FREE(xml); + + /* + * If the script raised an error abort the launch + */ + if (hookret < 0) + goto cleanup;
Should we also report an error if virHookCall returned 1 because the hook could not be run?
+ /* we can't stop the operation even if the script raised an error */ + virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name, + VIR_HOOK_QEMU_OP_STOPPED, VIR_HOOK_SUBOP_END, NULL, xml);
Likewise, should we report if virHookCall returns non-zero, even though we don't abort the operation? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Fri, Mar 26, 2010 at 11:09:08AM -0600, Eric Blake wrote:
On 03/26/2010 09:45 AM, Daniel Veillard wrote:
+ /* now that we know it is about to start call the hook if present */ + if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { + char *xml = virDomainDefFormat(vm->def, 0); + int hookret; + + hookret = virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name, + VIR_HOOK_QEMU_OP_START, VIR_HOOK_SUBOP_BEGIN, NULL, xml); + VIR_FREE(xml); + + /* + * If the script raised an error abort the launch + */ + if (hookret < 0) + goto cleanup;
Should we also report an error if virHookCall returned 1 because the hook could not be run?
I prefer no, that's a system setup error, and I'm afraid of this breaking everything. That's the danger of this cript extension, while errors reported by running the script must be taken into account, failing to run the script should not go in the way.
+ /* we can't stop the operation even if the script raised an error */ + virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name, + VIR_HOOK_QEMU_OP_STOPPED, VIR_HOOK_SUBOP_END, NULL, xml);
Likewise, should we report if virHookCall returns non-zero, even though we don't abort the operation?
Any system error would be reported from within virHookCall() itself, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Add script hook support to the LXC driver Right now this implements only 2 basic hooks: - before the lxc control process is being launched - after the lxc control process is terminated the XML description of the domain is passed to the hook script stdin /etc/libvirt/hook/lxc * src/lxc/lxc_driver.c: implement synchronous script hooks for LXC at domain startup and end diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index ba13065..1697363 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -49,6 +49,7 @@ #include "nodeinfo.h" #include "uuid.h" #include "stats_linux.h" +#include "hooks.h" #define VIR_FROM_THIS VIR_FROM_LXC @@ -722,6 +723,16 @@ static int lxcVmCleanup(lxc_driver_t *driver, DEBUG("container exited with rc: %d", rc); } + /* now that we know it's stopped call the hook if present */ + if (virHookPresent(VIR_HOOK_DRIVER_LXC)) { + char *xml = virDomainDefFormat(vm->def, 0); + + /* we can't stop the operation even if the script raised an error */ + virHookCall(VIR_HOOK_DRIVER_LXC, vm->def->name, + VIR_HOOK_LXC_OP_STOPPED, VIR_HOOK_SUBOP_END, NULL, xml); + VIR_FREE(xml); + } + virEventRemoveHandle(priv->monitorWatch); close(priv->monitor); @@ -1120,6 +1131,22 @@ static int lxcControllerStart(lxc_driver_t *driver, FD_SET(appPty, &keepfd); + /* now that we know it is about to start call the hook if present */ + if (virHookPresent(VIR_HOOK_DRIVER_LXC)) { + char *xml = virDomainDefFormat(vm->def, 0); + int hookret; + + hookret = virHookCall(VIR_HOOK_DRIVER_LXC, vm->def->name, + VIR_HOOK_LXC_OP_START, VIR_HOOK_SUBOP_BEGIN, NULL, xml); + VIR_FREE(xml); + + /* + * If the script raised an error abort the launch + */ + if (hookret < 0) + goto cleanup; + } + if (virExec(largv, lenv, &keepfd, &child, -1, &logfd, &logfd, VIR_EXEC_NONE) < 0) -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 03/26/2010 09:46 AM, Daniel Veillard wrote:
Add script hook support to the LXC driver
Right now this implements only 2 basic hooks: - before the lxc control process is being launched - after the lxc control process is terminated the XML description of the domain is passed to the hook script stdin /etc/libvirt/hook/lxc
Identical comments to the qemu 5/6 review on whether this is missing additional error reporting on failed calls to virHookCall. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Fri, Mar 26, 2010 at 04:32:41PM +0100, Daniel Veillard wrote:
The script are supposed to be small, execute fast as it's synchronous, their exit value is 0 otherwise it's considered a failure (which may be ignored but in general will stops the ongoing operation).
Okay, I have applied the changes suggested by Eric, rebased, added the missing entry in po/POTFILES.in and a few other cleanups, and then pushed this patch set, thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Mon, Mar 29, 2010 at 06:28:02PM +0200, Daniel Veillard wrote:
On Fri, Mar 26, 2010 at 04:32:41PM +0100, Daniel Veillard wrote:
The script are supposed to be small, execute fast as it's synchronous, their exit value is 0 otherwise it's considered a failure (which may be ignored but in general will stops the ongoing operation).
Okay, I have applied the changes suggested by Eric, rebased, added the missing entry in po/POTFILES.in and a few other cleanups, and then pushed this patch set,
Of course I made a mistake in those last changes, so I applied that trivial fix that Gerhard Stenzel raised on IRC Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (4)
-
Daniel P. Berrange -
Daniel Veillard -
Eric Blake -
Hugh O. Brock