From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44849) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uu235-0005pH-HH for qemu-devel@nongnu.org; Tue, 02 Jul 2013 10:59:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Uu230-0007OY-Iy for qemu-devel@nongnu.org; Tue, 02 Jul 2013 10:59:19 -0400 Received: from mail-qa0-x235.google.com ([2607:f8b0:400d:c00::235]:51107) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uu230-0007OT-Fg for qemu-devel@nongnu.org; Tue, 02 Jul 2013 10:59:14 -0400 Received: by mail-qa0-f53.google.com with SMTP id g10so2866535qah.5 for ; Tue, 02 Jul 2013 07:59:13 -0700 (PDT) MIME-Version: 1.0 Sender: flukshun@gmail.com In-Reply-To: <51D274DB.1080009@redhat.com> References: <51D274DB.1080009@redhat.com> From: Michael Roth Date: Tue, 2 Jul 2013 09:58:53 -0500 Message-ID: Content-Type: text/plain; charset=ISO-8859-1 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" , "stefanha@gmail.com" , "qemu-devel@nongnu.org" , "lcapitulino@redhat.com" , "vrozenfe@redhat.com" , Tomoki Sekiyama , "pbonzini@redhat.com" , Seiji Aguchi , "areis@redhat.com" On Tue, Jul 2, 2013 at 1:36 AM, 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. One thing to keep in mind is there are some failure paths in qmp_guest_fsfreeze_thaw(), for both win32 and posix, where we might not unset frozen state via ga_set_unfrozen(). In which case, logging will still be disabled. It might make sense to nest those error strings inside the one returned by qmp_guest_fsfreeze_freeze(), since that's the only reliable way to report it and the client will be interested in that information. This also makes introducing local_err immediately useful. > > Thanks, > Laszlo >