On 13.09.2016 02:21, Julio Faracco wrote:
Libvirtd only support hooks when the daemon is started. Hooks cannot be loaded when the daemon is already running. So, to load a hook you need to restart the service everytime. Now, the inotify support enables the option of create a hook and run it even if libvirtd was started.
Cc: Carlos Castilho <ccasti@br.ibm.com> Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
Interesting approach. The only thing I am worried about is that it might trick users into thinking we will implement something similar for editing XML files for domains. That is, if they edit /etc/libvirt/qemu/myFavouriteDomain.xml, libvirt will monitor the changes and re-read the file if needed.
--- daemon/libvirtd.c | 1 + src/libvirt_private.syms | 1 + src/util/virhook.c | 165 ++++++++++++++++++++++++++++++++++++++++++++++- src/util/virhook.h | 10 +++ 4 files changed, 175 insertions(+), 2 deletions(-)
The documentation is missing. There is docs/hooks.html.in file which covers this topic. Moreover, 'make all syntax-check check' is your friend ;-) I'm not gonna report problems found by syntax-check - now that you know we have such feature, you can fix those issues yourself.
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 95c1b1c..56175d1 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1622,6 +1622,7 @@ int main(int argc, char **argv) { 0, "shutdown", NULL, NULL);
cleanup: + virHookCleanUp(); virNetlinkEventServiceStopAll(); virObjectUnref(remoteProgram); virObjectUnref(lxcProgram); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6a77e46..c8ad816 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1648,6 +1648,7 @@ virHashValueFree; virHookCall; virHookInitialize; virHookPresent; +virHookCleanUp;
# util/virhostdev.h diff --git a/src/util/virhook.c b/src/util/virhook.c index facd74a..d5fc928 100644 --- a/src/util/virhook.c +++ b/src/util/virhook.c @@ -26,6 +26,7 @@ #include <sys/types.h> #include <sys/wait.h> #include <sys/stat.h> +#include <sys/inotify.h> #include <unistd.h> #include <stdlib.h> #include <stdio.h> @@ -45,6 +46,10 @@ VIR_LOG_INIT("util.hook");
#define LIBVIRT_HOOK_DIR SYSCONFDIR "/libvirt/hooks"
+#define virHookInstall(driver) virHooksFound |= (1 << driver); + +#define virHookUninstall(driver) virHooksFound ^= (1 << driver); + VIR_ENUM_DECL(virHookDriver) VIR_ENUM_DECL(virHookDaemonOp) VIR_ENUM_DECL(virHookSubop) @@ -109,6 +114,8 @@ VIR_ENUM_IMPL(virHookLibxlOp, VIR_HOOK_LIBXL_OP_LAST,
static int virHooksFound = -1;
+static virHookInotifyPtr virHooksInotify = NULL; + /** * virHookCheck: * @driver: the driver name "daemon", "qemu", "lxc"... @@ -153,6 +160,121 @@ virHookCheck(int no, const char *driver) return ret; }
+/** + * virHookInotifyEvent: + * @fd: inotify file descriptor. + * + * Identifies file events at libvirt's hook directory. + * Install or uninstall hooks on demand. Acording file manipulation. + */ +static void +virHookInotifyEvent(int watch ATTRIBUTE_UNUSED, + int fd, + int events ATTRIBUTE_UNUSED, + void *data ATTRIBUTE_UNUSED) +{ + char buf[1024];
man page suggests using slightly larger buffer.
+ struct inotify_event *e; + int got; + int driver; + char *tmp, *name; + + VIR_DEBUG("inotify event in virHookInotify()"); + +reread: + got = read(fd, buf, sizeof(buf));
@fd is blocking. We don't want to read from a blocking FD in our event loop (as it might block the whole event loop).
+ if (got == -1) { + if (errno == EINTR) + goto reread; + return; + }
We have a wrapper for that - saferead(). Also, it would be nice if an error is reported on failed read. Just imagine you're given the daemon logs with bug description 'libvirt hooks do not work with inotify'.
+ + tmp = buf; + while (got) { + if (got < sizeof(struct inotify_event)) + return; + + VIR_WARNINGS_NO_CAST_ALIGN + e = (struct inotify_event *)tmp; + VIR_WARNINGS_RESET + + tmp += sizeof(struct inotify_event); + got -= sizeof(struct inotify_event); + + if (got < e->len) + return;
Wait, what? This discards partially read event. Moreover, in the next iteration we start reading from wrong offset in the inotify_event struct and thus poison the well. I think we need to come up with better mechanism for reading events. Also - should we allocate receiving buffer for events dynamically on demand?
+ + tmp += e->len; + got -= e->len; + + name = (char *)&(e->name);
This does not feel right. e->name is type of ' char[]'.
+ + /* Removing hook file. */ + if (e->mask & (IN_DELETE | IN_MOVED_FROM)) { + if ((driver = virHookDriverTypeFromString(name)) < 0) { + VIR_DEBUG("Invalid hook name for %s", name); + return; + } + + virHookUninstall(driver); + } + + /* Creating hook file. */ + if (e->mask & (IN_CREATE | IN_CLOSE_WRITE | IN_MOVED_TO)) { + if ((driver = virHookDriverTypeFromString(name)) < 0) { + VIR_DEBUG("Invalid hook name for %s", name); + return; + } + + virHookInstall(driver); + } + } +}
Well, this just sets some internal variable. It doesn't reload the list of hooks or something. Moreover, these hooks are meant to be run at the daemon startup. Therefore I think you should stick to what I suggest in the other e-mail. Michal