Re: [Qemu-devel] [PATCH V3 4/4] block: dump snapshot and image info to specified output

2013-05-27 Thread Wenchao Xia 

于 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  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 


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);
?
--
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH V3 4/4] block: dump snapshot and image info to specified output

2013-05-27 Thread Luiz Capitulino 
On Mon, 27 May 2013 17:40:59 +0200
Kevin Wolf  wrote:

> Am 27.05.2013 um 17:02 hat Luiz Capitulino geschrieben:
> > On Sat, 25 May 2013 11:09:45 +0800
> > 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 
> > 
> > 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.

Re: [Qemu-devel] [PATCH V3 4/4] block: dump snapshot and image info to specified output

2013-05-27 Thread Kevin Wolf 
Am 27.05.2013 um 17:02 hat Luiz Capitulino geschrieben:
> On Sat, 25 May 2013 11:09:45 +0800
> 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 
> 
> 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

Re: [Qemu-devel] [PATCH V3 4/4] block: dump snapshot and image info to specified output

2013-05-27 Thread Luiz Capitulino 
On Sat, 25 May 2013 11:09:45 +0800
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 

I don't like the casting and the void pointers very much, but I can't
suggest anything better:

Reviewed-by: Luiz Capitulino 

> ---
>  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 / 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);
> +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",
> +

Re: [Qemu-devel] [PATCH V3 4/4] block: dump snapshot and image info to specified output

2013-05-25 Thread Eric Blake 
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 
> ---
>  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 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH V3 4/4] block: dump snapshot and image info to specified output

2013-05-24 Thread Wenchao Xia 
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 
---
 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 / 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);
+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 ope