From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:39263) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ufh7g-00021h-PN for qemu-devel@nongnu.org; Thu, 23 May 2013 21:48:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ufh7e-00039U-Qb for qemu-devel@nongnu.org; Thu, 23 May 2013 21:48:48 -0400 Received: from e23smtp06.au.ibm.com ([202.81.31.148]:52603) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ufh7e-00038k-8a for qemu-devel@nongnu.org; Thu, 23 May 2013 21:48:46 -0400 Received: from /spool/local by e23smtp06.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 24 May 2013 11:42:11 +1000 Received: from d23relay03.au.ibm.com (d23relay03.au.ibm.com [9.190.235.21]) by d23dlp01.au.ibm.com (Postfix) with ESMTP id B77DA2CE8052 for ; Fri, 24 May 2013 11:48:41 +1000 (EST) Received: from d23av02.au.ibm.com (d23av02.au.ibm.com [9.190.235.138]) by d23relay03.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r4O1mXhL22741242 for ; Fri, 24 May 2013 11:48:33 +1000 Received: from d23av02.au.ibm.com (loopback [127.0.0.1]) by d23av02.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r4O1me58013350 for ; Fri, 24 May 2013 11:48:40 +1000 Message-ID: <519EC6D1.6040605@linux.vnet.ibm.com> Date: Fri, 24 May 2013 09:48:01 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1369298836-17416-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1369298836-17416-6-git-send-email-xiawenc@linux.vnet.ibm.com> <519E3634.30107@redhat.com> In-Reply-To: <519E3634.30107@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH V2 5/5] block: dump to specified output for bdrv_snapshot_dump() and bdrv_image_info_dump() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: kwolf@redhat.com, phrdina@redhat.com, stefanha@gmail.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, pbonzini@redhat.com, armbru@redhat.com 于 2013-5-23 23:31, Eric Blake 写道: > On 05/23/2013 02:47 AM, Wenchao Xia wrote: >> Buffer is not used now so the string would not be truncated any more. They can be used >> by both qemu and qemu-img with correct parameter specified. >> >> Signed-off-by: Wenchao Xia >> --- >> block/qapi.c | 65 +++++++++++++++++++++++++++----------------------- >> include/block/qapi.h | 5 ++- >> qemu-img.c | 15 +++++++---- >> savevm.c | 11 ++++++-- >> 4 files changed, 55 insertions(+), 41 deletions(-) >> > >> @@ -282,17 +282,17 @@ char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn) >> (int)((secs / 60) % 60), >> (int)(secs % 60), >> (int)((sn->vm_clock_nsec / 1000000) % 1000)); >> - snprintf(buf, buf_size, >> - "%-10s%-20s%7s%20s%15s", >> - sn->id_str, sn->name, >> - get_human_readable_size(buf1, sizeof(buf1), sn->vm_state_size), >> - date_buf, >> - clock_buf); >> + message_printf(output, > > You got rid of ONE buffer... > >> + "%-10s%-20s%7s%20s%15s", >> + sn->id_str, sn->name, >> + get_human_readable_size(buf1, sizeof(buf1), > > ...but what is this other buffer still doing? get_human_readable_size > needs to be converted to use QemuOutput. > >> +void bdrv_image_info_dump(const QemuOutput *output, ImageInfo *info) >> { >> char size_buf[128], dsize_buf[128]; > > Why do we still need size_buf and dsize_buf? > >> if (!info->has_actual_size) { >> @@ -302,43 +302,47 @@ void bdrv_image_info_dump(ImageInfo *info) >> info->actual_size); >> } >> get_human_readable_size(size_buf, sizeof(size_buf), info->virtual_size); > > Again, get_human_readable_size should be converted to use QemuOutput. > They do not likely have a chance to be truncated, so have not change them. I will convert them also in next version. >> +++ b/qemu-img.c >> @@ -1554,16 +1554,18 @@ static void dump_snapshots(BlockDriverState *bs) >> { >> QEMUSnapshotInfo *sn_tab, *sn; >> int nb_sns, i; >> - char buf[256]; >> + QemuOutput output = {OUTPUT_STREAM, {stdout,} }; > > This is relying on C99's rule that a union is initialized by its first > named member. But I think it might be more readable as: > > output = { .kind = OUTPUT_STREAM, .stream = stdout }; > > not to mention that you will HAVE to use a designator to ever initialize > the monitor element of the union in any parallel code that favors the > monitor. This solve the initialization issue, will use it, thank u for the tip. > > Hmm, does C99 even allow anonymous unions, or is that a gcc extension? > > Overall, I like the direction this is headed. The conversion looks > reasonable, although it didn't quite go far enough for getting rid of > buffers. > -- Best Regards Wenchao Xia