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

[Markus Armbruster]

Eric Blake  writes:

> On 12/17/2015 09:49 AM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster 
>> ---
>>  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, >shm_size, );
>
> 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 

Thanks!

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

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
---
 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, >shm_size, );
 if (errp) {
-fprintf(stderr, "cannot parse shm size: %s\n",
-error_get_pretty(errp));
-error_free(errp);
+error_report_err(errp);
 ivshmem_server_usage(argv[0], 1);
 }
 break;
diff --git a/qdev-monitor.c b/qdev-monitor.c
index 30936df..3ce4710 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -266,8 +266,7 @@ int qdev_device_help(QemuOpts *opts)
 return 1;
 
 error:
-error_printf("%s\n", error_get_pretty(local_err));
-error_free(local_err);
+error_report_err(local_err);
 return 1;
 }
 
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 2881d84..ac7ceef 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -251,8 +251,7 @@ static void *nbd_client_thread(void *arg)
 , _error);
 if (ret < 0) {
 if (local_error) {
-fprintf(stderr, "%s\n", error_get_pretty(local_error));
-error_free(local_error);
+error_report_err(local_error);
 }
 goto out_socket;
 }
-- 
2.4.3

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

[Eric Blake]

On 12/17/2015 09:49 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster 
> ---
>  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, >shm_size, );

Idea for a followup patch: The name 'errp' is most often associated with
type 'Error **'; but here it is 'Error *', which is confusing.

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.

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature