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

[Peter Crosthwaite]

On Sun, May 11, 2014 at 11:27 PM, Le Tan <address@hidden> wrote:
> 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?

Yes sounds clear. Just leave those that aren't errors alone.

Give others a chance (a day or two) to weigh in on discussion as well.

Regards,
Peter

> Thanks very much!
>
> Regards,
> Le
>