On Sun, May 11, 2014 at 11:27 PM, Le Tan <tamlokv...@gmail.com> wrote:
> 2014-05-10 21:18 GMT+08:00 Peter Crosthwaite <peter.crosthwa...@xilinx.com>:
>> On Sat, May 10, 2014 at 9:55 AM, Le Tan <tamlokv...@gmail.com> 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 <tamlokv...@gmail.com>
>>> ---
>>> 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
>
