Re: [Qemu-devel] [PATCH v2 04/23] error: Use error_report_err() instead of ad hoc prints

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 12/17/2015 09:49 AM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  contrib/ivshmem-server/main.c | 4 +---
>>  qdev-monitor.c                | 3 +--
>>  qemu-nbd.c                    | 3 +--
>>  3 files changed, 3 insertions(+), 7 deletions(-)
>> 
>> diff --git a/contrib/ivshmem-server/main.c b/contrib/ivshmem-server/main.c
>> index 54ff001..00508b5 100644
>> --- a/contrib/ivshmem-server/main.c
>> +++ b/contrib/ivshmem-server/main.c
>> @@ -106,9 +106,7 @@ ivshmem_server_parse_args(IvshmemServerArgs *args, int 
>> argc, char *argv[])
>>          case 'l': /* shm_size */
>>              parse_option_size("shm_size", optarg, &args->shm_size, &errp);
>
> Idea for a followup patch: The name 'errp' is most often associated with
> type 'Error **'; but here it is 'Error *', which is confusing.

Yes.  Another round of these:

e940f54 qmp hmp: Consistently name Error * objects err, and not errp
2f719f1 hw: Consistently name Error * objects err, and not errp
4399c43 qemu-img: Consistently name Error * objects err, and not errp

> Other offenders in hmp.c, hw/core/nmi.c, include/qemu/sockets.h, and
> tests/test-string-output-visitor.c.
>
>>              if (errp) {
>> -                fprintf(stderr, "cannot parse shm size: %s\n",
>> -                        error_get_pretty(errp));
>> -                error_free(errp);
>> +                error_report_err(errp);
>
> This loses the "cannot parse shm size: " prefix; but I don't think that
> hurts.  Could use a mention in the commit message, though.

Losing the prefix is fine, because the error messages always mention the
parameter name.  For instance,

    cannot parse shm size: Parameter 'shm_size' expects a size

becomes

    Parameter 'shm_size' expects a size
    You may use k, M, G or T suffixes for kilobytes, megabytes, gigabytes and 
terabytes.

Note we now get the hint.

Including the error location would be even nicer, but is out of scope
for this series.

The program shows its complete usage help afterwards, which is annoying,
but out of scope for this series.

I'll amend the commit message.

> Reviewed-by: Eric Blake <address@hidden>

Thanks!