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

[Luiz Capitulino]

On Mon, 26 Nov 2012 20:49:08 +0900
Tomoki Sekiyama <address@hidden> 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 <address@hidden> 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 <address@hidden>
> >> ---
> >>  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,