From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:49708) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TWX0A-0003ry-3Y for qemu-devel@nongnu.org; Thu, 08 Nov 2012 13:38:55 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TWX08-0003D2-Ls for qemu-devel@nongnu.org; Thu, 08 Nov 2012 13:38:53 -0500 Received: from mx1.redhat.com ([209.132.183.28]:18538) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TWX08-0003Cy-DX for qemu-devel@nongnu.org; Thu, 08 Nov 2012 13:38:52 -0500 Message-ID: <509BFC36.80002@redhat.com> Date: Thu, 08 Nov 2012 11:38:46 -0700 From: Eric Blake MIME-Version: 1.0 References: <509B9FEF.4040604@hitachi.com> In-Reply-To: <509B9FEF.4040604@hitachi.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="------------enigFD1D3A12F7FF50A41AB7E841" 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: Tomoki Sekiyama Cc: qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enigFD1D3A12F7FF50A41AB7E841 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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] > 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. >=20 > When the qemu-ga receives fsfreeze-freeze command, the script specified= > in --fsfreeze-script option is executed with "freeze" argument before t= he > filesystem is frozen. For fsfreeze-thaw command, the script is executed= > with "thaw" argument after the filesystem is thawed. >=20 > Signed-off-by: Tomoki Sekiyama > --- > @@ -396,6 +397,34 @@ GuestFsfreezeStatus qmp_guest_fsfreeze_status(Erro= r **err) > return GUEST_FSFREEZE_STATUS_THAWED; > } > =20 > +int execute_fsfreeze_script(const char *arg) > +{ > + int ret =3D -1; > + const char *fsfreeze_script; > + char *cmdline; > + struct stat st; > + > + fsfreeze_script =3D ga_fsfreeze_script(ga_state); > + if (fsfreeze_script && stat(fsfreeze_script, &st) =3D=3D 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? > + slog("executing fsfreeze script with arg `%s'", arg); > + cmdline =3D malloc(strlen(fsfreeze_script) + strlen(arg) += 2); Don't we prefer g_malloc over malloc in qemu-ga? > + if (cmdline) { > + sprintf(cmdline, "%s %s", fsfreeze_script, arg); Thankfully, this doesn't overflow, but isn't there a glib function that makes it easier to create a malloc'd concatenated string in one call rather than pairing a malloc and sprintf? That said, why do you even need a single string, given that... > + ret =3D system(cmdline); =2E..system() is not required to be thread-safe, but we should assume tha= t 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(). > + free(cmdline); > + } > + if (ret > 0) { > + g_warning("fsfreeze script failed with status=3D%d", r= et); 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. > + } else if (ret =3D=3D -1) { > + g_warning("execution of fsfreeze script failed: %s", > + strerror(errno)); free() is allowed to clobber errno, which means you may be reporting the wrong error if system() failed with -1. > + } > + } > + } > + return ret; > +} > + 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. --=20 Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --------------enigFD1D3A12F7FF50A41AB7E841 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (GNU/Linux) Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Mozilla - http://www.enigmail.net/ iQEcBAEBCAAGBQJQm/w2AAoJEKeha0olJ0NqDREIAKKMv0wuqPdNEwgWjnFUI2ih vYV/Bz998ICqVHLFd8k7cg4/f4wES6SWiJzWV/0k7WDGLHDQ0Wf+PPkBfOZ3jFe8 ewyxlWveolhhpYyzGsNCNbGK8pDiI6+XUv+Xll7Both6Ajek1KxOtfmS6VdJi/KE Yasxj6tdqiCoAEQgVcmiToXKbRG2SOytuZfk/ZtJFSk8YqpglI7Xy/LfjtKK7Jr7 NEymcIaXsXkh9G9TZKR4dzdgWtrI8HPpRa1KcIzCi2aDNxinT96fcRIkNOGvIoR1 /mAMFEvxn4baciBX2i90yByVAoTWVG7D/CJrykCeNvpV01mR67jjfvswKh+M/oc= =tWnb -----END PGP SIGNATURE----- --------------enigFD1D3A12F7FF50A41AB7E841--