Re: [Qemu-devel] [PATCH v3 11/24] error: Use error_reportf_err() where it makes obvious sense

[Eric Blake]

On 12/18/2015 08:35 AM, Markus Armbruster wrote:
> Done with this Coccinelle semantic patch
> 
> @@
> expression FMT, E, S;
> expression list ARGS;
> @@
> -error_report(FMT, ARGS, error_get_pretty(E));
> +error_reportf_err(E, FMT/*@@@*/, ARGS);
> (
> -error_free(E);
> |
>exit(S);
> |
>abort();
> )
> 
> followed by a replace of '%s"/*@@@*/' by '"' and some line rewrapping,
> because I can't figure out how to make Coccinelle transform strings.
> 
> We now use the error whole instead of just its message obtained with
> error_get_pretty().  This avoids suppressing its hint (see commit
> 50b7b00), but I can't see how the errors touched in this commit could
> come with hints.
> 
> Signed-off-by: Markus Armbruster 
> ---

> +++ b/arch_init.c
> @@ -258,9 +258,7 @@ void do_acpitable_option(const QemuOpts *opts)
>  
>  acpi_table_add(opts, );
>  if (err) {
> -error_report("Wrong acpi table provided: %s",
> - error_get_pretty(err));
> -error_free(err);
> +error_reportf_err(err, "Wrong acpi table provided: ");

Bikeshedding: should error_reportf_err() automatically add the trailing
": " to the prefix, instead of having every caller express it?  Would
affect 10/24 as well.  But I can't see a strong reason to add the churn
it would cause for a respin, so I won't insist.

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

Re: [Qemu-devel] [PATCH v3 11/24] error: Use error_reportf_err() where it makes obvious sense

[Markus Armbruster]

Eric Blake  writes:

> On 12/18/2015 08:35 AM, Markus Armbruster wrote:
>> Done with this Coccinelle semantic patch
>> 
>> @@
>> expression FMT, E, S;
>> expression list ARGS;
>> @@
>> -error_report(FMT, ARGS, error_get_pretty(E));
>> +error_reportf_err(E, FMT/*@@@*/, ARGS);
>> (
>> -error_free(E);
>> |
>>   exit(S);
>> |
>>   abort();
>> )
>> 
>> followed by a replace of '%s"/*@@@*/' by '"' and some line rewrapping,
>> because I can't figure out how to make Coccinelle transform strings.
>> 
>> We now use the error whole instead of just its message obtained with
>> error_get_pretty().  This avoids suppressing its hint (see commit
>> 50b7b00), but I can't see how the errors touched in this commit could
>> come with hints.
>> 
>> Signed-off-by: Markus Armbruster 
>> ---
>
>> +++ b/arch_init.c
>> @@ -258,9 +258,7 @@ void do_acpitable_option(const QemuOpts *opts)
>>  
>>  acpi_table_add(opts, );
>>  if (err) {
>> -error_report("Wrong acpi table provided: %s",
>> - error_get_pretty(err));
>> -error_free(err);
>> +error_reportf_err(err, "Wrong acpi table provided: ");
>
> Bikeshedding: should error_reportf_err() automatically add the trailing
> ": " to the prefix, instead of having every caller express it?  Would
> affect 10/24 as well.  But I can't see a strong reason to add the churn
> it would cause for a respin, so I won't insist.

I decided not to add ": " automatically, to make the prefix-ness of the
argument a bit more obvious.

> Reviewed-by: Eric Blake 

Thanks!

[Qemu-devel] [PATCH v3 11/24] error: Use error_reportf_err() where it makes obvious sense

[Markus Armbruster]

Done with this Coccinelle semantic patch

@@
expression FMT, E, S;
expression list ARGS;
@@
-error_report(FMT, ARGS, error_get_pretty(E));
+error_reportf_err(E, FMT/*@@@*/, ARGS);
(
-error_free(E);
|
 exit(S);
|
 abort();
)

followed by a replace of '%s"/*@@@*/' by '"' and some line rewrapping,
because I can't figure out how to make Coccinelle transform strings.

We now use the error whole instead of just its message obtained with
error_get_pretty().  This avoids suppressing its hint (see commit
50b7b00), but I can't see how the errors touched in this commit could
come with hints.

Signed-off-by: Markus Armbruster 
---
 arch_init.c   |  4 +---
 block/sheepdog.c  |  5 ++---
 blockdev.c| 12 +---
 hw/arm/cubieboard.c   |  9 -
 hw/arm/digic_boards.c |  3 +--
 hw/core/qdev-properties.c |  6 ++
 hw/core/qdev.c|  5 ++---
 hw/i386/pc.c  |  5 ++---
 hw/ppc/e500.c |  4 ++--
 hw/usb/bus.c  |  5 ++---
 qemu-img.c| 33 +
 qemu-nbd.c| 11 +--
 replay/replay.c   |  3 +--
 tests/test-aio.c  |  4 +---
 tests/test-thread-pool.c  |  4 +---
 ui/vnc.c  |  4 +---
 vl.c  |  6 ++
 17 files changed, 47 insertions(+), 76 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 38f5fb9..d1383b3 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -258,9 +258,7 @@ void do_acpitable_option(const QemuOpts *opts)
 
 acpi_table_add(opts, );
 if (err) {
-error_report("Wrong acpi table provided: %s",
- error_get_pretty(err));
-error_free(err);
+error_reportf_err(err, "Wrong acpi table provided: ");
 exit(1);
 }
 #endif
diff --git a/block/sheepdog.c b/block/sheepdog.c
index dd8301b..6986be8 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2405,9 +2405,8 @@ static int sd_snapshot_create(BlockDriverState *bs, 
QEMUSnapshotInfo *sn_info)
 
 ret = do_sd_create(s, _vid, 1, _err);
 if (ret < 0) {
-error_report("failed to create inode for snapshot: %s",
- error_get_pretty(local_err));
-error_free(local_err);
+error_reportf_err(local_err,
+  "failed to create inode for snapshot: ");
 goto cleanup;
 }
 
diff --git a/blockdev.c b/blockdev.c
index 13eaa77..f93a6ee 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1583,13 +1583,11 @@ static void internal_snapshot_abort(BlkActionState 
*common)
 }
 
 if (bdrv_snapshot_delete(bs, sn->id_str, sn->name, _error) < 0) {
-error_report("Failed to delete snapshot with id '%s' and name '%s' on "
- "device '%s' in abort: %s",
- sn->id_str,
- sn->name,
- bdrv_get_device_name(bs),
- error_get_pretty(local_error));
-error_free(local_error);
+error_reportf_err(local_error,
+  "Failed to delete snapshot with id '%s' and "
+  "name '%s' on device '%s' in abort: ",
+  sn->id_str, sn->name,
+  bdrv_get_device_name(bs));
 }
 }
 
diff --git a/hw/arm/cubieboard.c b/hw/arm/cubieboard.c
index bf068cd..a71e43c 100644
--- a/hw/arm/cubieboard.c
+++ b/hw/arm/cubieboard.c
@@ -39,27 +39,26 @@ static void cubieboard_init(MachineState *machine)
 
 object_property_set_int(OBJECT(>a10->emac), 1, "phy-addr", );
 if (err != NULL) {
-error_report("Couldn't set phy address: %s", error_get_pretty(err));
+error_reportf_err(err, "Couldn't set phy address: ");
 exit(1);
 }
 
 object_property_set_int(OBJECT(>a10->timer), 32768, "clk0-freq", );
 if (err != NULL) {
-error_report("Couldn't set clk0 frequency: %s", error_get_pretty(err));
+error_reportf_err(err, "Couldn't set clk0 frequency: ");
 exit(1);
 }
 
 object_property_set_int(OBJECT(>a10->timer), 2400, "clk1-freq",
 );
 if (err != NULL) {
-error_report("Couldn't set clk1 frequency: %s", error_get_pretty(err));
+error_reportf_err(err, "Couldn't set clk1 frequency: ");
 exit(1);
 }
 
 object_property_set_bool(OBJECT(s->a10), true, "realized", );
 if (err != NULL) {
-error_report("Couldn't realize Allwinner A10: %s",
- error_get_pretty(err));
+error_reportf_err(err, "Couldn't realize Allwinner A10: ");
 exit(1);
 }
 
diff --git a/hw/arm/digic_boards.c b/hw/arm/digic_boards.c
index 710045a..dfaed25 100644
--- a/hw/arm/digic_boards.c
+++ b/hw/arm/digic_boards.c
@@ -64,8 +64,7 @@ static void digic4_board_init(DigicBoard *board)
 s->digic = DIGIC(object_new(TYPE_DIGIC));