Re: [Qemu-devel] [PATCH v4 1/2] qemu-ga: execute hook to quiesce the guest on fsfreeze-freeze/thaw

[Luiz Capitulino <lcapitulino@redhat.com>]

On Thu, 22 Nov 2012 11:15:46 +0900
Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com> wrote:

> To use the online disk snapshot for online-backup, application-level
> consistency of the snapshot image is required. However, currently the
> guest agent can provide only filesystem-level consistency, and the
> snapshot may contain dirty data, for example, incomplete transactions.
> This patch provides the opportunity to quiesce applications before
> snapshot is taken.
> 
> When the qemu-ga receives fsfreeze-freeze command, the hook script
> specified in --fsfreeze-hook option is executed with "freeze" argument
> before the filesystem is frozen. For fsfreeze-thaw command, the hook
> script is executed with "thaw" argument after the filesystem is thawed.
> 
> Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com>
> ---
>  qemu-ga.c              |   42 ++++++++++++++++++++++++++++++-
>  qga/commands-posix.c   |   66 ++++++++++++++++++++++++++++++++++++++++++++++++
>  qga/guest-agent-core.h |    1 +
>  3 files changed, 108 insertions(+), 1 deletion(-)
> 
> diff --git a/qemu-ga.c b/qemu-ga.c
> index 9b59a52..240f6e2 100644
> --- a/qemu-ga.c
> +++ b/qemu-ga.c
> @@ -34,6 +34,12 @@
>  #include "qga/service-win32.h"
>  #include <windows.h>
>  #endif
> +#ifdef __linux__
> +#include <linux/fs.h>
> +#ifdef FIFREEZE
> +#define CONFIG_FSFREEZE
> +#endif
> +#endif
>  
>  #ifndef _WIN32
>  #define QGA_VIRTIO_PATH_DEFAULT "/dev/virtio-ports/org.qemu.guest_agent.0"
> @@ -42,6 +48,9 @@
>  #endif
>  #define QGA_STATEDIR_DEFAULT CONFIG_QEMU_LOCALSTATEDIR "/run"
>  #define QGA_PIDFILE_DEFAULT QGA_STATEDIR_DEFAULT "/qemu-ga.pid"
> +#ifdef CONFIG_FSFREEZE
> +#define QGA_FSFREEZE_HOOK_DEFAULT CONFIG_QEMU_CONFDIR "/fsfreeze-hook"
> +#endif
>  #define QGA_SENTINEL_BYTE 0xFF
>  
>  struct GAState {
> @@ -64,6 +73,9 @@ struct GAState {
>          const char *log_filepath;
>          const char *pid_filepath;
>      } deferred_options;
> +#ifdef CONFIG_FSFREEZE
> +    const char *fsfreeze_hook;
> +#endif
>  };
>  
>  struct GAState *ga_state;
> @@ -153,6 +165,10 @@ static void usage(const char *cmd)
>  "                    %s)\n"
>  "  -l, --logfile     set logfile path, logs to stderr by default\n"
>  "  -f, --pidfile     specify pidfile (default is %s)\n"
> +#ifdef CONFIG_FSFREEZE
> +"  -F, --fsfreeze-hook\n"
> +"                    specify fsfreeze hook (default is %s)\n"
> +#endif
>  "  -t, --statedir    specify dir to store state information (absolute paths\n"
>  "                    only, default is %s)\n"
>  "  -v, --verbose     log extra debugging information\n"
> @@ -167,6 +183,9 @@ static void usage(const char *cmd)
>  "\n"
>  "Report bugs to <mdroth@linux.vnet.ibm.com>\n"
>      , cmd, QEMU_VERSION, QGA_VIRTIO_PATH_DEFAULT, QGA_PIDFILE_DEFAULT,
> +#ifdef CONFIG_FSFREEZE
> +    QGA_FSFREEZE_HOOK_DEFAULT,
> +#endif
>      QGA_STATEDIR_DEFAULT);
>  }
>  
> @@ -401,6 +420,13 @@ void ga_unset_frozen(GAState *s)
>      }
>  }
>  
> +#ifdef CONFIG_FSFREEZE
> +const char *ga_fsfreeze_hook(GAState *s)
> +{

Argument can be const.

> +    return s->fsfreeze_hook;
> +}
> +#endif
> +
>  static void become_daemon(const char *pidfile)
>  {
>  #ifndef _WIN32
> @@ -678,10 +704,13 @@ VOID WINAPI service_main(DWORD argc, TCHAR *argv[])
>  
>  int main(int argc, char **argv)
>  {
> -    const char *sopt = "hVvdm:p:l:f:b:s:t:";
> +    const char *sopt = "hVvdm:p:l:f:F:b:s:t:";
>      const char *method = NULL, *path = NULL;
>      const char *log_filepath = NULL;
>      const char *pid_filepath = QGA_PIDFILE_DEFAULT;
> +#ifdef CONFIG_FSFREEZE
> +    const char *fsfreeze_hook = QGA_FSFREEZE_HOOK_DEFAULT;
> +#endif
>      const char *state_dir = QGA_STATEDIR_DEFAULT;
>  #ifdef _WIN32
>      const char *service = NULL;
> @@ -691,6 +720,9 @@ int main(int argc, char **argv)
>          { "version", 0, NULL, 'V' },
>          { "logfile", 1, NULL, 'l' },
>          { "pidfile", 1, NULL, 'f' },
> +#ifdef CONFIG_FSFREEZE
> +        { "fsfreeze-hook", 1, NULL, 'F' },
> +#endif
>          { "verbose", 0, NULL, 'v' },
>          { "method", 1, NULL, 'm' },
>          { "path", 1, NULL, 'p' },
> @@ -723,6 +755,11 @@ int main(int argc, char **argv)
>          case 'f':
>              pid_filepath = optarg;
>              break;
> +#ifdef CONFIG_FSFREEZE
> +        case 'F':
> +            fsfreeze_hook = optarg;
> +            break;
> +#endif
>          case 't':
>               state_dir = optarg;
>               break;
> @@ -786,6 +823,9 @@ int main(int argc, char **argv)
>      s = g_malloc0(sizeof(GAState));
>      s->log_level = log_level;
>      s->log_file = stderr;
> +#ifdef CONFIG_FSFREEZE
> +    s->fsfreeze_hook = fsfreeze_hook;
> +#endif
>      g_log_set_default_handler(ga_log, s);
>      g_log_set_fatal_mask(NULL, G_LOG_LEVEL_ERROR);
>      ga_enable_logging(s);
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 726930a..9849c10 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -396,6 +396,62 @@ GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **err)
>      return GUEST_FSFREEZE_STATUS_THAWED;
>  }
>  
> +typedef enum {
> +    FSFREEZE_HOOK_THAW = 0,
> +    FSFREEZE_HOOK_FREEZE,
> +} FsfreezeHookArg;
> +
> +const char *fsfreeze_hook_arg_string[] = {
> +    "thaw",
> +    "freeze",
> +};
> +
> +/*
> + * Return -1 if hook is configured and exited abnormally. Otherwise return 0.
> + */
> +static int execute_fsfreeze_hook(FsfreezeHookArg arg)

Why don't you allow the argument to be the execle()'s "arg" string, as it's
only used by execle() itself? This way you can drop the enum and the array
pointer.

> +{
> +    int status;
> +    pid_t pid, rpid;
> +    const char *hook;
> +    const char *arg_str = fsfreeze_hook_arg_string[arg];
> +
> +    hook = ga_fsfreeze_hook(ga_state);
> +    if (!hook || access(hook, X_OK) != 0) {

Can hook be NULL? If it can't, then this should be an assert(), although
I think it would be nice to allow the hook to be disabled.

Also, you shouldn't silently fail if the execution bit is not set. At a
minimum you should emit an warning. I guess I would return an error though.

> +        return 0;
> +    }
> +
> +    slog("executing fsfreeze hook with arg `%s'", arg_str);
> +    pid = fork();
> +    if (pid == 0) {
> +        setsid();
> +        reopen_fd_to_null(0);
> +        reopen_fd_to_null(1);
> +        reopen_fd_to_null(2);
> +
> +        execle(hook, hook, arg_str, NULL, environ);
> +        _exit(EXIT_FAILURE);
> +    } else if (pid < 0) {
> +        slog("execution of fsfreeze hook failed: %s", strerror(errno));
> +        return -1;

Please, return a good error here. You could change this function
to take an Error object and do:

error_setg_errno(err, errno, "failed to wait for child (pid: %d)", pid);

> +    }
> +
> +    do {
> +        rpid = waitpid(pid, &status, 0);
> +    } while (rpid == -1 && errno == EINTR);

I was refactoring error messages & handling in qemu-ga and added a
function called ga_wait_child(), so that we avoid duplicating the
loop above and also have good error reporting.

You can check it here and grab it if you want:

http://repo.or.cz/w/qemu/qmp-unstable.git/commitdiff/036e1cb5a3d9f8d797c7220e049fa1e0042df4dc

Also note that you're ignoring waitpid() failures != EINTR.

> +    if (rpid == pid && WIFEXITED(status)) {
> +        int st = WEXITSTATUS(status);
> +        if (st) {
> +            slog("fsfreeze hook failed with status %d", st);

We need this on a error message back to the client otherwise it will
be harder to debug freeze failures (ie. use error_setg() here).

> +            return -1;
> +        }
> +    } else if (rpid == pid && WIFSIGNALED(status)) {
> +        slog("fsfreeze hook killed by signal %d", WTERMSIG(status));
> +        return -1;
> +    }

If pid > 0 and waitpid() didn't fail, then rpid == pid.

> +    return 0;
> +}
> +
>  /*
>   * Walk list of mounted file systems in the guest, and freeze the ones which
>   * are real local file systems.
> @@ -410,6 +466,13 @@ int64_t qmp_guest_fsfreeze_freeze(Error **err)
>  
>      slog("guest-fsfreeze called");
>  
> +    ret = execute_fsfreeze_hook(FSFREEZE_HOOK_FREEZE);
> +    if (ret < 0) {
> +        sprintf(err_msg, "execution of fsfreeze hook failed");
> +        error_set(err, QERR_QGA_COMMAND_FAILED, err_msg);
> +        return ret;

If you propagate errors from execute_fsfreeze_hook() you can drop all this
and fix the generic error message.

> +    }
> +
>      QTAILQ_INIT(&mounts);
>      ret = build_fs_mount_list(&mounts);
>      if (ret < 0) {
> @@ -513,6 +576,9 @@ int64_t qmp_guest_fsfreeze_thaw(Error **err)
>  
>      ga_unset_frozen(ga_state);
>      free_fs_mount_list(&mounts);
> +
> +    execute_fsfreeze_hook(FSFREEZE_HOOK_THAW);
> +
>      return i;
>  }
>  
> diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
> index 49a7abe..c6e8de0 100644
> --- a/qga/guest-agent-core.h
> +++ b/qga/guest-agent-core.h
> @@ -34,6 +34,7 @@ void ga_set_response_delimited(GAState *s);
>  bool ga_is_frozen(GAState *s);
>  void ga_set_frozen(GAState *s);
>  void ga_unset_frozen(GAState *s);
> +const char *ga_fsfreeze_hook(GAState *s);
>  
>  #ifndef _WIN32
>  void reopen_fd_to_null(int fd);
> 
>