From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39565) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UtiNn-0006nm-Mm for qemu-devel@nongnu.org; Mon, 01 Jul 2013 13:59:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UtiNj-0000Oa-Uw for qemu-devel@nongnu.org; Mon, 01 Jul 2013 13:59:23 -0400 Received: from usindpps05.hds.com ([207.126.252.18]:59039) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UtiNj-0000OB-NE for qemu-devel@nongnu.org; Mon, 01 Jul 2013 13:59:19 -0400 From: Tomoki Sekiyama Date: Mon, 1 Jul 2013 17:59:10 +0000 Message-ID: In-Reply-To: <51D18426.5010909@redhat.com> Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-ID: Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH v4 08/10] qemu-ga: call Windows VSS requester in fsfreeze command handler List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laszlo Ersek Cc: "libaiqing@huawei.com" , "mdroth@linux.vnet.ibm.com" , "stefanha@gmail.com" , "qemu-devel@nongnu.org" , "lcapitulino@redhat.com" , "vrozenfe@redhat.com" , "pbonzini@redhat.com" , Seiji Aguchi , "areis@redhat.com" On 7/1/13 9:29 , "Laszlo Ersek" wrote: >>+error: >> + qmp_guest_fsfreeze_thaw(NULL); > >Passing NULL here as "errp" concerns me slightly. I've been assuming >that "errp" is never NULL due to the outermost QMP layer always wanting >to receive errors and to serialize them. > >Specifically, a NULL "errp" would turn all error_set*(errp, ...) calls >into no-ops under qmp_guest_fsfreeze_thaw(), and all error_is_set(errp) >questions would answer with false. That said, nothing seems to be >affected right now. > >Maybe a dummy local variable would be more future-proof... OTOH it would >be stylistically questionable :) OK, then it should be: Error *local_err =3D NULL; ... error: qmp_guest_fsfreeze_thaw(&local_err); if (error_is_set(&local_err)) { error_free(local_err); } >>+#ifdef HAS_VSS_SDK >> +#include "qga/vss-win32-provider.h" >> +#include "qga/vss-win32-requester.h" >> +#endif > >The protection of #include "qga/vss-win32-requester.h" is inconsistent >between "commands-win32.c" and "qga/main.c". For now the header only >brings in a few prototypes, which shouldn't cause trouble in >"commands-win32.c" even without VSS. Still consistent guarding would be >nice. I will remove #ifdef's from qga/main.c. Instead, I will add static inline functions that does nothing when CONFIG_VSS_SDK isn't defined. (And vss-win32-provider.h can also be removed by stop linking the DLL.) >... This patch made me look at ga_command_state_cleanup_all(). >Apparently the POSIX flavor thaws filesystems if qemu-ga exits before a >thaw command over QMP; see guest_fsfreeze_cleanup() and >ga_command_state_init(). Do you think something similar would be useful >for the Windows flavor as well? Hm, however the backup is automatically terminated if IVssBackupComponents is released, it would be nice to have cleanup. I will add them. Thanks, Tomoki Sekiyama