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

[Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com>]

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,
-- 
Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com>
Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory