Hi Michael, Thanks for your review.
On 2012/11/21 6:00, mdroth wrote: > On Tue, Nov 13, 2012 at 01:56:56PM +0900, Tomoki Sekiyama 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...@hitachi.com> >> --- <snip> >> 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" > > Small nit, but can we change this to --fsfreeze-hook? Ah, sure. >> diff --git a/qga/commands-posix.c b/qga/commands-posix.c >> index 726930a..8c3e341 100644 >> --- a/qga/commands-posix.c >> +++ b/qga/commands-posix.c >> @@ -396,6 +396,45 @@ GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error >> **err) >> return GUEST_FSFREEZE_STATUS_THAWED; >> } >> > > Rather than passing freeze/thaw in as a string, I think an enum > parameter of the form: > > typedef enum { > FSFREEZE_HOOK_THAW, > FSFREEZE_HOOK_FREEZE, > } FsfreezeHook; > > would be better in terms of documenting the interface. Okey, I will use enum as the argument. >> +static void execute_fsfreeze_hook(const char *arg) <snip> >> /* >> * Walk list of mounted file systems in the guest, and freeze the ones which >> * are real local file systems. >> @@ -410,6 +449,8 @@ int64_t qmp_guest_fsfreeze_freeze(Error **err) >> >> slog("guest-fsfreeze called"); >> >> + execute_fsfreeze_hook("freeze"); >> + > > I think if a user configures a pre-freeze hook, it's failure should be > treated as a failure of the fsfreeze call in general, otherwise we risk > moving forward based on false assumptions that can lead to data loss or > other issues. I think the thaw hook is okay the way it is though, they > can review the logs for any issues arising after the VM is > unfrozen. I think that is reasonable. It would be better to notify the failure of fsfreeze hook to the host side for investigation of the issue. I will fix these in the next patchset. Thanks, -- Tomoki Sekiyama <tomoki.sekiyama...@hitachi.com> Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory