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

[Le Tan]

2014-05-10 21:18 GMT+08:00 Peter Crosthwaite <address@hidden>:
> 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
Hi! I am a novice and this is my warm-up task for GSoC. So you mean
that it's good to convert printfs to error_report where the message is
deemed to notice the user, such as some warning about wrong cmd
arguments and so on, while it is better to let them be where the
printfs are used for the developer to debug, such as:
#if defined(DEBUG_NBD)
#define logout(fmt, ...) \
    fprintf(stderr, "nbd\t%-24s" fmt, __func__, ##__VA_ARGS__)

Am I right?
I think I should fix the patch and convert the printfs to error_report
selectively. Do I get the idea?
Thanks very much!

Regards,
Le