From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54014) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uu2T6-0000RU-Oo for qemu-devel@nongnu.org; Tue, 02 Jul 2013 11:26:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Uu2T3-00069I-ML for qemu-devel@nongnu.org; Tue, 02 Jul 2013 11:26:12 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53702) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uu2T3-00068w-FU for qemu-devel@nongnu.org; Tue, 02 Jul 2013 11:26:09 -0400 Message-ID: <51D2F197.6020704@redhat.com> Date: Tue, 02 Jul 2013 17:28:23 +0200 From: Laszlo Ersek MIME-Version: 1.0 References: <51D274DB.1080009@redhat.com> <20130702100903.43435654@redhat.com> <51D2E767.9010106@redhat.com> <20130702111659.507c705c@redhat.com> In-Reply-To: <20130702111659.507c705c@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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: Luiz Capitulino Cc: "libaiqing@huawei.com" , "stefanha@gmail.com" , "qemu-devel@nongnu.org" , "mdroth@linux.vnet.ibm.com" , "vrozenfe@redhat.com" , Tomoki Sekiyama , "pbonzini@redhat.com" , Seiji Aguchi , "areis@redhat.com" On 07/02/13 17:16, Luiz Capitulino wrote: > On Tue, 02 Jul 2013 16:44:55 +0200 > Laszlo Ersek wrote: > >> On 07/02/13 16:09, Luiz Capitulino wrote: >>> On Tue, 02 Jul 2013 08:36:11 +0200 >>> Laszlo Ersek wrote: >>> >>>> On 07/01/13 19:59, Tomoki Sekiyama wrote: >>>>> 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 = NULL; >>>>> ... >>>>> error: >>>>> qmp_guest_fsfreeze_thaw(&local_err); >>>>> if (error_is_set(&local_err)) { >>>>> error_free(local_err); >>>>> } >>>> >>>> I think so, yes. >>>> >>>> You could also log it before freeing it I guess: >>>> >>>> g_debug("cleanup thaw: %s", error_get_pretty(local_err)); >>>> >>>> or some such, but it's probably not important. >>> >>> I'd rather do something like that, otherwise it doesn't seem to >>> make sense to pass Error as qmp_guest_fsfreeze_thaw() does >>> use a local Error. >> >> No, the win32 version of qmp_guest_fsfreeze_thaw() being added by this >> patch doesn't. > > Oh, I looked into the POSIX one. But then, it's qmp_guest_fsfreeze_thaw() > that has to be fixed, isn't it? I didn't insist on that because the QMP command implementations in the guest agent are all rooted in the dispatcher, and the dispatcher makes sure for all commands that errp is never NULL. This is the only call site where a NULL errp was manually passed, and we're discussing how to fix it -- use a local_err, and maybe even print it. Of course I don't insist on *not* reworking it either. Thanks, Laszlo