[Qemu-devel] [PATCH V2 5/5] block: dump to specified output for bdrv_snapshot_dump() and bdrv_image_info_dump()
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(-) diff --git a/block/qapi.c b/block/qapi.c index 155e77e..2e083f8 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -259,7 +259,7 @@ static char *get_human_readable_size(char *buf, int buf_size, int64_t size) return buf; } -char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn) +void bdrv_snapshot_dump(const QemuOutput *output, QEMUSnapshotInfo *sn) { char buf1[128], date_buf[128], clock_buf[128]; struct tm tm; @@ -267,9 +267,9 @@ char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn) int64_t secs; if (!sn) { -snprintf(buf, buf_size, - "%-10s%-20s%7s%20s%15s", - "ID", "TAG", "VM SIZE", "DATE", "VM CLOCK"); +message_printf(output, + "%-10s%-20s%7s%20s%15s", + "ID", "TAG", "VM SIZE", "DATE", "VM CLOCK"); } else { ti = sn->date_sec; localtime_r(&ti, &tm); @@ -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 / 100) % 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, + "%-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); } -return buf; } -void bdrv_image_info_dump(ImageInfo *info) +void bdrv_image_info_dump(const QemuOutput *output, ImageInfo *info) { char size_buf[128], dsize_buf[128]; 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); -printf("image: %s\n" - "file format: %s\n" - "virtual size: %s (%" PRId64 " bytes)\n" - "disk size: %s\n", - info->filename, info->format, size_buf, - info->virtual_size, - dsize_buf); +message_printf(output, + "image: %s\n" + "file format: %s\n" + "virtual size: %s (%" PRId64 " bytes)\n" + "disk size: %s\n", + info->filename, info->format, size_buf, + info->virtual_size, + dsize_buf); if (info->has_encrypted && info->encrypted) { -printf("encrypted: yes\n"); +message_printf(output, "encrypted: yes\n"); } if (info->has_cluster_size) { -printf("cluster_size: %" PRId64 "\n", info->cluster_size); +message_printf(output, "cluster_size: %" PRId64 "\n", + info->cluster_size); } if (info->has_dirty_flag && info->dirty_flag) { -printf("cleanly shut down: no\n"); +message_printf(output, "cleanly shut down: no\n"); } if (info->has_backing_filename) { -printf("backing file: %s", info->backing_filename); +message_printf(output, "backing file: %s", info->backing_filename); if (info->has_full_backing_filename) { -printf(" (actual path: %s)", info->full_backing_filename); +message_printf(output, " (actual path: %s)", + info->full_backing_filename); } -putchar('\n'); +message_printf(output, "\n"); if (info->has_backing_filename_format) { -printf("backing file format: %s\n", info->backing_filename_format); +message_printf(output, "backing file format: %s\n", + info->backing_filename_format); } } if (info->has_snapshots) { SnapshotInfoList *elem; -char buf[256]; -printf("Snapshot list:\n"); -printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL)); +message_printf(output, "Snapshot list:\n"); +bdrv_snapshot_dump(output, NULL); +message_printf(output, "\n"); /* Ideally bdrv_snapshot_dump() would operate on SnapshotInfoList but * we convert to the block layer's native QEMUSnapshotInfo for now. @@ -
Re: [Qemu-devel] [PATCH V2 5/5] block: dump to specified output for bdrv_snapshot_dump() and bdrv_image_info_dump()
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 / 100) % 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. > +++ 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. 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. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH V2 5/5] block: dump to specified output for bdrv_snapshot_dump() and bdrv_image_info_dump()
于 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 / 100) % 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
Re: [Qemu-devel] [PATCH V2 5/5] block: dump to specified output for bdrv_snapshot_dump() and bdrv_image_info_dump()
于 2013-5-24 9:48, Wenchao Xia 写道: 于 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 / 100) % 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. Just found this buffer is used in format control: message_printf(output, "%-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); if get_human_readable_size() is converted, a manual control is needed. Same thing happens to clock_buf. It seems better to keep what it is. +++ 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