From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:46268) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UQGG2-0001YQ-Op for qemu-devel@nongnu.org; Thu, 11 Apr 2013 08:05:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UQGFy-0001PK-5n for qemu-devel@nongnu.org; Thu, 11 Apr 2013 08:05:38 -0400 Received: from mx1.redhat.com ([209.132.183.28]:2216) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UQGFx-0001PA-Va for qemu-devel@nongnu.org; Thu, 11 Apr 2013 08:05:34 -0400 From: Markus Armbruster References: <1364903250-10429-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1364903250-10429-14-git-send-email-xiawenc@linux.vnet.ibm.com> <87fvyytixe.fsf@blackfin.pond.sub.org> <516659A9.3010101@linux.vnet.ibm.com> Date: Thu, 11 Apr 2013 14:05:29 +0200 In-Reply-To: <516659A9.3010101@linux.vnet.ibm.com> (Wenchao Xia's message of "Thu, 11 Apr 2013 14:35:21 +0800") Message-ID: <87fvyxff1y.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH V11 13/17] block: dump to buffer for bdrv_snapshot_dump() and bdrv_image_info_dump() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wenchao Xia Cc: kwolf@redhat.com, stefanha@gmail.com, lcapitulino@redhat.com, qemu-devel@nongnu.org, pbonzini@redhat.com Wenchao Xia writes: >>> >>> -void bdrv_image_info_dump(ImageInfo *info) >>> +void bdrv_image_info_dump(GString *buf, ImageInfo *info) >>> { >>> char size_buf[128], dsize_buf[128]; >>> if (!info->has_actual_size) { >>> @@ -370,43 +369,48 @@ void bdrv_image_info_dump(ImageInfo *info) >> >> I don't like this change, because it introduces buffering for no >> discernible reason. Unless you can show me one, I'd like you to keep >> printing directly. >> > HMP code later need to call this function, and then print buf to > monitor console, which is the goal of this patch. Aha, the actual problem is "print either to stdout or to current monitor", so that we can share code among qemu-img and monitor commands. Such sharing is good, and the problem is real. bdrv_snapshot_dump() solves it this way: print to a fixed-size buffer, which the caller either prints to stdout (qemu-img) or to the current monitor (info snapshots). Your patch makes the buffer flexible. Improvement. But in my opinion, the detour through the buffer is silly. Why not simply use a print function that does the right thing? We already have such a function for "print either to stderr or to current monitor": error_printf(). Could easily add one for stdout. [...]