Patchwork [v4,1/2] qemu-ga: execute hook to quiesce the guest on fsfreeze-freeze/thaw

login
register
mail settings
Submitter Tomoki Sekiyama
Date Nov. 22, 2012, 2:15 a.m.
Message ID <20121122021546.9852.30043.stgit@melchior2.sdl.hitachi.co.jp>
Download mbox | patch
Permalink /patch/200924/
State New
Headers show

Comments

Tomoki Sekiyama - Nov. 22, 2012, 2:15 a.m.
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(-)
Luiz Capitulino - Nov. 22, 2012, 3:52 p.m.
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);
> 
>
Tomoki Sekiyama - Nov. 26, 2012, 11:49 a.m.
Hi, thanks for your review.

On 2012/11/23 0:52, Luiz Capitulino wrote:
> 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(-)
<snip>
>> @@ -401,6 +420,13 @@ void ga_unset_frozen(GAState *s)
>>      }
>>  }
>>  
>> +#ifdef CONFIG_FSFREEZE
>> +const char *ga_fsfreeze_hook(GAState *s)
>> +{
> 
> Argument can be const.

OK, should be fixed at next patch.

>> --- 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.

This is based on Michael's comment that enum style is better in terms of
documenting the interface.

>> +{
>> +    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.

Currently whether the hook is executed is determined by whether it is
installed at default path (or specified path by the option).
I think it is easier to configure, but it could make the hook disabled
by default and enable it only if --fsfreeze-hook option is specified.
How do you think of that?

> 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.

I agree.

>> +        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);

OK, I will try this.

>> +    }
>> +
>> +    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

Thanks for the information. I will check your patch.
I also inspect the issues below at the same time.

> 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.

Thanks,
Luiz Capitulino - Nov. 26, 2012, 12:32 p.m.
On Mon, 26 Nov 2012 20:49:08 +0900
Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com> wrote:

> Hi, thanks for your review.
> 
> On 2012/11/23 0:52, Luiz Capitulino wrote:
> > 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(-)
> <snip>
> >> @@ -401,6 +420,13 @@ void ga_unset_frozen(GAState *s)
> >>      }
> >>  }
> >>  
> >> +#ifdef CONFIG_FSFREEZE
> >> +const char *ga_fsfreeze_hook(GAState *s)
> >> +{
> > 
> > Argument can be const.
> 
> OK, should be fixed at next patch.
> 
> >> --- 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.
> 
> This is based on Michael's comment that enum style is better in terms of
> documenting the interface.

Well, I don't fully agree but I'm not strong about this either.

> >> +{
> >> +    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.
> 
> Currently whether the hook is executed is determined by whether it is
> installed at default path (or specified path by the option).

Yes.

> I think it is easier to configure, but it could make the hook disabled
> by default and enable it only if --fsfreeze-hook option is specified.
> How do you think of that?

I agree that the way you coded it makes it easier to the user, just
two comments:

 1. As I noted below, you should issue a warning if the hook file
    doesn't exist or doesn't have the execution bit set

 2. There should be a way to disable it at the command-line. Tap scripts
    for example can be disabled by specifying "no" as the script path, like
	script=no. We could do that for qemu-ga too.

> 
> > 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.
> 
> I agree.
> 
> >> +        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);
> 
> OK, I will try this.
> 
> >> +    }
> >> +
> >> +    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
> 
> Thanks for the information. I will check your patch.
> I also inspect the issues below at the same time.

Well, I'm going to rebase this series and will probably post it later
today or tomorrow. So, you could rebase on top of it or just forget about
my comment above so that we avoid conflicts.

> 
> > 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.
> 
> Thanks,

Patch

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)
+{
+    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)
+{
+    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) {
+        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;
+    }
+
+    do {
+        rpid = waitpid(pid, &status, 0);
+    } while (rpid == -1 && errno == EINTR);
+    if (rpid == pid && WIFEXITED(status)) {
+        int st = WEXITSTATUS(status);
+        if (st) {
+            slog("fsfreeze hook failed with status %d", st);
+            return -1;
+        }
+    } else if (rpid == pid && WIFSIGNALED(status)) {
+        slog("fsfreeze hook killed by signal %d", WTERMSIG(status));
+        return -1;
+    }
+    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;
+    }
+
     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);