| Message ID | 1369451385-23452-5-git-send-email-xiawenc@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 05/24/2013 09:09 PM, Wenchao Xia wrote: > bdrv_snapshot_dump() and bdrv_image_info_dump() do not dump to a buffer now, > some internal buffers are still used for format control, which have no > chance to be truncated. As a result, these two functions have no more issue > of truncation, and they can be used by both qemu and qemu-img with correct > parameter specified. > > Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> > --- > block/qapi.c | 66 +++++++++++++++++++++++++++---------------------- > include/block/qapi.h | 6 +++- > qemu-img.c | 9 ++++--- > savevm.c | 7 +++-- > 4 files changed, 49 insertions(+), 39 deletions(-) > -char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn) > +void bdrv_snapshot_dump(fprintf_function func_fprintf, void *f, > + QEMUSnapshotInfo *sn) > { > char buf1[128], date_buf[128], clock_buf[128]; > + func_fprintf(f, > + "%-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); Now that 'buf' is no longer in scope, it might be nice to rename 'buf1' to something more meaningful; maybe size_buf to go along with the other two named buffers. But the choice of naming doesn't impact the correctness, so Reviewed-by: Eric Blake <eblake@redhat.com>
On Sat, 25 May 2013 11:09:45 +0800 Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote: > bdrv_snapshot_dump() and bdrv_image_info_dump() do not dump to a buffer now, > some internal buffers are still used for format control, which have no > chance to be truncated. As a result, these two functions have no more issue > of truncation, and they can be used by both qemu and qemu-img with correct > parameter specified. > > Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> I don't like the casting and the void pointers very much, but I can't suggest anything better: Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com> > --- > block/qapi.c | 66 +++++++++++++++++++++++++++---------------------- > include/block/qapi.h | 6 +++- > qemu-img.c | 9 ++++--- > savevm.c | 7 +++-- > 4 files changed, 49 insertions(+), 39 deletions(-) > > diff --git a/block/qapi.c b/block/qapi.c > index 155e77e..794dbf8 100644 > --- a/block/qapi.c > +++ b/block/qapi.c > @@ -259,7 +259,8 @@ 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(fprintf_function func_fprintf, void *f, > + QEMUSnapshotInfo *sn) > { > char buf1[128], date_buf[128], clock_buf[128]; > struct tm tm; > @@ -267,9 +268,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"); > + func_fprintf(f, > + "%-10s%-20s%7s%20s%15s", > + "ID", "TAG", "VM SIZE", "DATE", "VM CLOCK"); > } else { > ti = sn->date_sec; > localtime_r(&ti, &tm); > @@ -282,17 +283,18 @@ 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); > + func_fprintf(f, > + "%-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(fprintf_function func_fprintf, void *f, > + ImageInfo *info) > { > char size_buf[128], dsize_buf[128]; > if (!info->has_actual_size) { > @@ -302,43 +304,46 @@ 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); > + func_fprintf(f, > + "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"); > + func_fprintf(f, "encrypted: yes\n"); > } > > if (info->has_cluster_size) { > - printf("cluster_size: %" PRId64 "\n", info->cluster_size); > + func_fprintf(f, "cluster_size: %" PRId64 "\n", > + info->cluster_size); > } > > if (info->has_dirty_flag && info->dirty_flag) { > - printf("cleanly shut down: no\n"); > + func_fprintf(f, "cleanly shut down: no\n"); > } > > if (info->has_backing_filename) { > - printf("backing file: %s", info->backing_filename); > + func_fprintf(f, "backing file: %s", info->backing_filename); > if (info->has_full_backing_filename) { > - printf(" (actual path: %s)", info->full_backing_filename); > + func_fprintf(f, " (actual path: %s)", info->full_backing_filename); > } > - putchar('\n'); > + func_fprintf(f, "\n"); > if (info->has_backing_filename_format) { > - printf("backing file format: %s\n", info->backing_filename_format); > + func_fprintf(f, "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)); > + func_fprintf(f, "Snapshot list:\n"); > + bdrv_snapshot_dump(func_fprintf, f, NULL); > + func_fprintf(f, "\n"); > > /* Ideally bdrv_snapshot_dump() would operate on SnapshotInfoList but > * we convert to the block layer's native QEMUSnapshotInfo for now. > @@ -354,7 +359,8 @@ void bdrv_image_info_dump(ImageInfo *info) > > pstrcpy(sn.id_str, sizeof(sn.id_str), elem->value->id); > pstrcpy(sn.name, sizeof(sn.name), elem->value->name); > - printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), &sn)); > + bdrv_snapshot_dump(func_fprintf, f, &sn); > + func_fprintf(f, "\n"); > } > } > } > diff --git a/include/block/qapi.h b/include/block/qapi.h > index 55d1848..e6e568d 100644 > --- a/include/block/qapi.h > +++ b/include/block/qapi.h > @@ -36,6 +36,8 @@ void bdrv_collect_image_info(BlockDriverState *bs, > BlockInfo *bdrv_query_info(BlockDriverState *s); > BlockStats *bdrv_query_stats(const BlockDriverState *bs); > > -char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn); > -void bdrv_image_info_dump(ImageInfo *info); > +void bdrv_snapshot_dump(fprintf_function func_fprintf, void *f, > + QEMUSnapshotInfo *sn); > +void bdrv_image_info_dump(fprintf_function func_fprintf, void *f, > + ImageInfo *info); > #endif > diff --git a/qemu-img.c b/qemu-img.c > index 5d1e480..82c7977 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -1554,16 +1554,17 @@ static void dump_snapshots(BlockDriverState *bs) > { > QEMUSnapshotInfo *sn_tab, *sn; > int nb_sns, i; > - char buf[256]; > > nb_sns = bdrv_snapshot_list(bs, &sn_tab); > if (nb_sns <= 0) > return; > printf("Snapshot list:\n"); > - printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL)); > + bdrv_snapshot_dump(fprintf, stdout, NULL); > + printf("\n"); > for(i = 0; i < nb_sns; i++) { > sn = &sn_tab[i]; > - printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), sn)); > + bdrv_snapshot_dump(fprintf, stdout, sn); > + printf("\n"); > } > g_free(sn_tab); > } > @@ -1613,7 +1614,7 @@ static void dump_human_image_info_list(ImageInfoList *list) > } > delim = true; > > - bdrv_image_info_dump(elem->value); > + bdrv_image_info_dump(fprintf, stdout, elem->value); > } > } > > diff --git a/savevm.c b/savevm.c > index f988e89..9b9d037 100644 > --- a/savevm.c > +++ b/savevm.c > @@ -2540,7 +2540,6 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict) > int nb_sns, i, ret, available; > int total; > int *available_snapshots; > - char buf[256]; > > bs = find_vmstate_bs(); > if (!bs) { > @@ -2583,10 +2582,12 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict) > } > > if (total > 0) { > - monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL)); > + bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, NULL); > + monitor_printf(mon, "\n"); > for (i = 0; i < total; i++) { > sn = &sn_tab[available_snapshots[i]]; > - monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), sn)); > + bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, sn); > + monitor_printf(mon, "\n"); > } > } else { > monitor_printf(mon, "There is no suitable snapshot available\n");
Am 27.05.2013 um 17:02 hat Luiz Capitulino geschrieben: > On Sat, 25 May 2013 11:09:45 +0800 > Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote: > > > bdrv_snapshot_dump() and bdrv_image_info_dump() do not dump to a buffer now, > > some internal buffers are still used for format control, which have no > > chance to be truncated. As a result, these two functions have no more issue > > of truncation, and they can be used by both qemu and qemu-img with correct > > parameter specified. > > > > Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> > > I don't like the casting and the void pointers very much, but I can't > suggest anything better: Maybe introduce and use a different function pointer type that explicitly takes void* instead of FILE*. You'd still have to cast the function pointers (and now actually in all instances), but then at least nobody can accidentally misinterpret a Monitor* as FILE*. Kevin
On Mon, 27 May 2013 17:40:59 +0200 Kevin Wolf <kwolf@redhat.com> wrote: > Am 27.05.2013 um 17:02 hat Luiz Capitulino geschrieben: > > On Sat, 25 May 2013 11:09:45 +0800 > > Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote: > > > > > bdrv_snapshot_dump() and bdrv_image_info_dump() do not dump to a buffer now, > > > some internal buffers are still used for format control, which have no > > > chance to be truncated. As a result, these two functions have no more issue > > > of truncation, and they can be used by both qemu and qemu-img with correct > > > parameter specified. > > > > > > Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> > > > > I don't like the casting and the void pointers very much, but I can't > > suggest anything better: > > Maybe introduce and use a different function pointer type that > explicitly takes void* instead of FILE*. You'd still have to cast the > function pointers (and now actually in all instances), but then at least > nobody can accidentally misinterpret a Monitor* as FILE*. I'm not sure this helps much because we'd have two functions pointers to choose (which fails the purpose of having the function pointer IMO) and also, there's code in QEMU doing this cast/void dance already. If we want a good solution, we'd have to find a better way to print to the monitor and to standard output.
δΊ 2013-5-27 23:40, Kevin Wolf ει: > Am 27.05.2013 um 17:02 hat Luiz Capitulino geschrieben: >> On Sat, 25 May 2013 11:09:45 +0800 >> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote: >> >>> bdrv_snapshot_dump() and bdrv_image_info_dump() do not dump to a buffer now, >>> some internal buffers are still used for format control, which have no >>> chance to be truncated. As a result, these two functions have no more issue >>> of truncation, and they can be used by both qemu and qemu-img with correct >>> parameter specified. >>> >>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> >> >> I don't like the casting and the void pointers very much, but I can't >> suggest anything better: > > Maybe introduce and use a different function pointer type that > explicitly takes void* instead of FILE*. You'd still have to cast the > function pointers (and now actually in all instances), but then at least > nobody can accidentally misinterpret a Monitor* as FILE*. > > Kevin > You mean change typedef int (*fprintf_function)(FILE *f, const char *fmt, ...) GCC_FMT_ATTR(2, 3); to typedef int (*fprintf_function)(void *out, const char *fmt, ...) GCC_FMT_ATTR(2, 3); ?
diff --git a/block/qapi.c b/block/qapi.c index 155e77e..794dbf8 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -259,7 +259,8 @@ 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(fprintf_function func_fprintf, void *f, + QEMUSnapshotInfo *sn) { char buf1[128], date_buf[128], clock_buf[128]; struct tm tm; @@ -267,9 +268,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"); + func_fprintf(f, + "%-10s%-20s%7s%20s%15s", + "ID", "TAG", "VM SIZE", "DATE", "VM CLOCK"); } else { ti = sn->date_sec; localtime_r(&ti, &tm); @@ -282,17 +283,18 @@ 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); + func_fprintf(f, + "%-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(fprintf_function func_fprintf, void *f, + ImageInfo *info) { char size_buf[128], dsize_buf[128]; if (!info->has_actual_size) { @@ -302,43 +304,46 @@ 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); + func_fprintf(f, + "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"); + func_fprintf(f, "encrypted: yes\n"); } if (info->has_cluster_size) { - printf("cluster_size: %" PRId64 "\n", info->cluster_size); + func_fprintf(f, "cluster_size: %" PRId64 "\n", + info->cluster_size); } if (info->has_dirty_flag && info->dirty_flag) { - printf("cleanly shut down: no\n"); + func_fprintf(f, "cleanly shut down: no\n"); } if (info->has_backing_filename) { - printf("backing file: %s", info->backing_filename); + func_fprintf(f, "backing file: %s", info->backing_filename); if (info->has_full_backing_filename) { - printf(" (actual path: %s)", info->full_backing_filename); + func_fprintf(f, " (actual path: %s)", info->full_backing_filename); } - putchar('\n'); + func_fprintf(f, "\n"); if (info->has_backing_filename_format) { - printf("backing file format: %s\n", info->backing_filename_format); + func_fprintf(f, "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)); + func_fprintf(f, "Snapshot list:\n"); + bdrv_snapshot_dump(func_fprintf, f, NULL); + func_fprintf(f, "\n"); /* Ideally bdrv_snapshot_dump() would operate on SnapshotInfoList but * we convert to the block layer's native QEMUSnapshotInfo for now. @@ -354,7 +359,8 @@ void bdrv_image_info_dump(ImageInfo *info) pstrcpy(sn.id_str, sizeof(sn.id_str), elem->value->id); pstrcpy(sn.name, sizeof(sn.name), elem->value->name); - printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), &sn)); + bdrv_snapshot_dump(func_fprintf, f, &sn); + func_fprintf(f, "\n"); } } } diff --git a/include/block/qapi.h b/include/block/qapi.h index 55d1848..e6e568d 100644 --- a/include/block/qapi.h +++ b/include/block/qapi.h @@ -36,6 +36,8 @@ void bdrv_collect_image_info(BlockDriverState *bs, BlockInfo *bdrv_query_info(BlockDriverState *s); BlockStats *bdrv_query_stats(const BlockDriverState *bs); -char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn); -void bdrv_image_info_dump(ImageInfo *info); +void bdrv_snapshot_dump(fprintf_function func_fprintf, void *f, + QEMUSnapshotInfo *sn); +void bdrv_image_info_dump(fprintf_function func_fprintf, void *f, + ImageInfo *info); #endif diff --git a/qemu-img.c b/qemu-img.c index 5d1e480..82c7977 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -1554,16 +1554,17 @@ static void dump_snapshots(BlockDriverState *bs) { QEMUSnapshotInfo *sn_tab, *sn; int nb_sns, i; - char buf[256]; nb_sns = bdrv_snapshot_list(bs, &sn_tab); if (nb_sns <= 0) return; printf("Snapshot list:\n"); - printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL)); + bdrv_snapshot_dump(fprintf, stdout, NULL); + printf("\n"); for(i = 0; i < nb_sns; i++) { sn = &sn_tab[i]; - printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), sn)); + bdrv_snapshot_dump(fprintf, stdout, sn); + printf("\n"); } g_free(sn_tab); } @@ -1613,7 +1614,7 @@ static void dump_human_image_info_list(ImageInfoList *list) } delim = true; - bdrv_image_info_dump(elem->value); + bdrv_image_info_dump(fprintf, stdout, elem->value); } } diff --git a/savevm.c b/savevm.c index f988e89..9b9d037 100644 --- a/savevm.c +++ b/savevm.c @@ -2540,7 +2540,6 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict) int nb_sns, i, ret, available; int total; int *available_snapshots; - char buf[256]; bs = find_vmstate_bs(); if (!bs) { @@ -2583,10 +2582,12 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict) } if (total > 0) { - monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL)); + bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, NULL); + monitor_printf(mon, "\n"); for (i = 0; i < total; i++) { sn = &sn_tab[available_snapshots[i]]; - monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), sn)); + bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, sn); + monitor_printf(mon, "\n"); } } else { monitor_printf(mon, "There is no suitable snapshot available\n");
bdrv_snapshot_dump() and bdrv_image_info_dump() do not dump to a buffer now, some internal buffers are still used for format control, which have no chance to be truncated. As a result, these two functions have no more issue of truncation, and they can be used by both qemu and qemu-img with correct parameter specified. Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- block/qapi.c | 66 +++++++++++++++++++++++++++---------------------- include/block/qapi.h | 6 +++- qemu-img.c | 9 ++++--- savevm.c | 7 +++-- 4 files changed, 49 insertions(+), 39 deletions(-)