Hi Eric, thanks for your review. On 2012/11/09 3:38, Eric Blake wrote: > On 11/08/2012 05:05 AM, Tomoki Sekiyama wrote: > > [Recoding to UTF-8, as ISO-2022-JP is not universally installed these > days - you may want to reconsider your mailer's defaults]
Now this should be UTF-8, sorry. >> 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 script specified >> in --fsfreeze-script option is executed with "freeze" argument before the >> filesystem is frozen. For fsfreeze-thaw command, the script is executed >> with "thaw" argument after the filesystem is thawed. >> >> Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama...@hitachi.com> >> --- > >> @@ -396,6 +397,34 @@ GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error >> **err) >> return GUEST_FSFREEZE_STATUS_THAWED; >> } >> >> +int execute_fsfreeze_script(const char *arg) >> +{ >> + int ret = -1; >> + const char *fsfreeze_script; >> + char *cmdline; >> + struct stat st; >> + >> + fsfreeze_script = ga_fsfreeze_script(ga_state); >> + if (fsfreeze_script && stat(fsfreeze_script, &st) == 0) { >> + if (S_ISREG(st.st_mode) && (st.st_mode & S_IXUSR)) { > > Is it any simpler to use access(fsfreeze_script, X_OK) to check if the > script exists and is executable? OK, I will use access() here. <snip> >> + ret = system(cmdline); > > ...system() is not required to be thread-safe, but we should assume that > qemu-ga is multi-threaded, and therefore we should not use system. > Besides executing things via an extra layer of shell opens doors for > further problems; for example, if the user starts qemu-ga with > --fsfreeze-script '/path/with spaces/script', your command line is > horribly broken when passed through the shell. It would be much better > to directly fork() and exec() the script ourselves instead of relying on > system(). I will use fork()/exec() method instead of system(), as in shutdown, so I can remove malloc/free. >> + if (ret > 0) { >> + g_warning("fsfreeze script failed with status=%d", ret); > > This is a potentially misleading message; you should be using macros > such as WEXITSTATUS when interpreting the result of system(), since not > all systems return exit status 1 in the same bit position. OK. I will refine error messages. > The idea of having the freeze and thaw actions hook out to > user-specified actions for additional steps seems nice, but this patch > series needs a lot more work. Regards, -- Tomoki Sekiyama <tomoki.sekiyama...@hitachi.com> Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory