Re: [Qemu-devel] [PATCH 3/4] block: replace fprintf(stderr, ...) with error_report() in block/

[Peter Crosthwaite]

On Sat, May 10, 2014 at 9:55 AM, Le Tan <address@hidden> wrote:
> Replace fprintf(stderr,...) with error_report() in files block/*, block.c,
> block-migration.c and blockdev.c. The trailing "\n"s of the @fmt argument
> have been removed because @fmt of error_report() should not contain newline.
>
> Signed-off-by: Le Tan <address@hidden>
> ---
>  block-migration.c      |    4 +-
>  block.c                |    4 +-
>  block/linux-aio.c      |    4 +-
>  block/nbd-client.h     |    2 +-
>  block/qcow2-cluster.c  |    4 +-
>  block/qcow2-refcount.c |  114 
> ++++++++++++++++++++++++------------------------
>  block/qcow2.c          |   16 +++----
>  block/quorum.c         |    4 +-
>  block/raw-posix.c      |    8 ++--
>  block/raw-win32.c      |    6 +--
>  block/ssh.c            |    4 +-
>  block/vdi.c            |   14 +++---
>  block/vmdk.c           |   21 ++++-----
>  block/vpc.c            |    4 +-
>  block/vvfat.c          |  108 ++++++++++++++++++++++-----------------------
>  blockdev.c             |    6 +--
>  16 files changed, 160 insertions(+), 163 deletions(-)
>
> diff --git a/block-migration.c b/block-migration.c
> index 56951e0..5bcf7c8 100644
> --- a/block-migration.c
> +++ b/block-migration.c
> @@ -790,7 +790,7 @@ static int block_load(QEMUFile *f, void *opaque, int 
> version_id)
>
>              bs = bdrv_find(device_name);
>              if (!bs) {
> -                fprintf(stderr, "Error unknown block device %s\n",
> +                error_report("Error unknown block device %s",
>                          device_name);
>                  return -EINVAL;
>              }
> @@ -833,7 +833,7 @@ static int block_load(QEMUFile *f, void *opaque, int 
> version_id)
>                     (addr == 100) ? '\n' : '\r');
>              fflush(stdout);
>          } else if (!(flags & BLK_MIG_FLAG_EOS)) {
> -            fprintf(stderr, "Unknown block migration flags: %#x\n", flags);
> +            error_report("Unknown block migration flags: %#x", flags);
>              return -EINVAL;
>          }
>          ret = qemu_file_get_error(f);
> diff --git a/block.c b/block.c
> index b749d31..7dc4acb 100644
> --- a/block.c
> +++ b/block.c
> @@ -2694,8 +2694,8 @@ static int bdrv_prwv_co(BlockDriverState *bs, int64_t 
> offset,
>       * if it has been enabled.
>       */
>      if (bs->io_limits_enabled) {
> -        fprintf(stderr, "Disabling I/O throttling on '%s' due "
> -                        "to synchronous I/O.\n", bdrv_get_device_name(bs));
> +        error_report("Disabling I/O throttling on '%s' due "
> +                     "to synchronous I/O.", bdrv_get_device_name(bs));
>          bdrv_io_limits_disable(bs);
>      }
>
> diff --git a/block/linux-aio.c b/block/linux-aio.c
> index 53434e2..b706a59 100644
> --- a/block/linux-aio.c
> +++ b/block/linux-aio.c
> @@ -162,8 +162,8 @@ BlockDriverAIOCB *laio_submit(BlockDriverState *bs, void 
> *aio_ctx, int fd,
>         break;
>      /* Currently Linux kernel does not support other operations */
>      default:
> -        fprintf(stderr, "%s: invalid AIO request type 0x%x.\n",
> -                        __func__, type);
> +        error_report("%s: invalid AIO request type 0x%x.",
> +                     __func__, type);
>          goto out_free_aiocb;
>      }
>      io_set_eventfd(&laiocb->iocb, event_notifier_get_fd(&s->e));
> diff --git a/block/nbd-client.h b/block/nbd-client.h
> index f2a6337..74178f4 100644
> --- a/block/nbd-client.h
> +++ b/block/nbd-client.h
> @@ -9,7 +9,7 @@
>
>  #if defined(DEBUG_NBD)
>  #define logout(fmt, ...) \
> -    fprintf(stderr, "nbd\t%-24s" fmt, __func__, ##__VA_ARGS__)
> +    error_report("nbd\t%-24s" fmt, __func__, ##__VA_ARGS__)

So i'm not sure we want to convert debug printfery to error_report.
There's very good value in converting the printfs with user
visibility, but ones like this seem intended for developers only as
throwaway-output. My thinking is that this is a lower level output
than error_report. For instance, as a developer you may do something
to break your error API yet you still want your debug printfery.
Wouldn't matter in this location, but there may be other parts of the
tree where we don't want error_report relinace for debug
instrumentation and it just seems better to keep it consistent.

Thinking further afield, qemu_log may ultimately be the correct
mechanism for this instead (I think thats what I have been using for
new code recently anyway).

Thoughts from others?

Regards,
Peter