[libvirt] [PATCH RFC] virhook: Adding inotify support to virhook.

[Michal Privoznik]

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 at br.ibm.com>
> Signed-off-by: Julio Faracco <jcfaracco at 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