From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:40049) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TXlLJ-0004L8-DV for qemu-devel@nongnu.org; Sun, 11 Nov 2012 23:09:52 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TXlLG-00060M-BS for qemu-devel@nongnu.org; Sun, 11 Nov 2012 23:09:49 -0500 Received: from mail7.hitachi.co.jp ([133.145.228.42]:44684) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TXlLF-0005zE-S4 for qemu-devel@nongnu.org; Sun, 11 Nov 2012 23:09:46 -0500 Message-ID: <50A076A9.50704@hitachi.com> Date: Mon, 12 Nov 2012 13:10:17 +0900 From: Tomoki Sekiyama MIME-Version: 1.0 References: <509B9FEF.4040604@hitachi.com> <509BFC36.80002@redhat.com> In-Reply-To: <509BFC36.80002@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/2] qemu-ga: execute script to quiesce the guest on fsfreeze-freeze/thaw List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: eblake@redhat.com Cc: qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com 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 >> --- > >> @@ -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. >> + 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 Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory