From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45824) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a49E6-00032g-BN for qemu-devel@nongnu.org; Wed, 02 Dec 2015 10:21:54 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a49E0-0001sX-Ld for qemu-devel@nongnu.org; Wed, 02 Dec 2015 10:21:50 -0500 Received: from mx1.redhat.com ([209.132.183.28]:49501) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a49E0-0001sQ-GB for qemu-devel@nongnu.org; Wed, 02 Dec 2015 10:21:44 -0500 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id EC396694 for ; Wed, 2 Dec 2015 15:21:43 +0000 (UTC) Date: Wed, 2 Dec 2015 23:21:36 +0800 From: Peter Xu Message-ID: <20151202152134.GA18589@pxdev.xzpeter.org> References: <1448976530-15984-1-git-send-email-peterx@redhat.com> <1448976530-15984-9-git-send-email-peterx@redhat.com> <20151202011131.GE9399@ad.usersys.redhat.com> <565F0420.10409@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <565F0420.10409@redhat.com> Subject: Re: [Qemu-devel] [PATCH v4 08/11] dump-guest-memory: add qmp event DUMP_COMPLETED List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: drjones@redhat.com, Fam Zheng , armbru@redhat.com, qemu-devel@nongnu.org, pbonzini@redhat.com, lcapitulino@redhat.com, lersek@redhat.com On Wed, Dec 02, 2015 at 07:45:52AM -0700, Eric Blake wrote: > On 12/01/2015 06:11 PM, Fam Zheng wrote: > > Please explicitly mention that successful dump emits DUMP_COMPLETED without > > error, and failed dump emits DUMP_COMPLETED that has an error str. > > In fact, I wonder if it would also be worth having a > 'status':'DumpStatus' field, which records the final status of the dump > (either 'completed' or 'failed'), and which is always present. Will the raw memory total size useful in any way? I am totally ok to add this, just failed to find a way for user to use it besides calculating finished work during dump... :( > > > >> +++ b/util/error.c > >> @@ -197,7 +197,11 @@ ErrorClass error_get_class(const Error *err) > >> > >> const char *error_get_pretty(Error *err) > >> { > >> - return err->msg; > >> + if (err) { > >> + return err->msg; > >> + } else { > >> + return NULL; > >> + } > > > > This change belongs to a separate patch, if any. > > Indeed. When I was musing about the idea, I was not expecting you to > actually implement it, so much as questioning whether it is a worthwhile > idea. But as it impacts more than just your series, it definitely needs > to be a separate patch, if at all. Sorry for the misunderstanding. I will make sure to use a new patch when needed next time. For this change, I will revert it and take Fam's latter suggestion to avoid modifying error.c. Thanks. Peter > > > But personally I don't like > > it, because it doesn't work very well when error_get_pretty is used in > > printf-like function parameters: > > > > Error *err = NULL; > > error_report("error: %s", error_get_pretty(err)); > > > > will print "error: (null)" which is ugly, > > Or even segfault. glibc is nice for printing "(null)", but the behavior > is undefined by POSIX and other libc aren't as nice as glibc. And that > was not a consequence I thought about when first raising the question of > whether it was even worth changing the contract of error_get_pretty(). > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org >