* Re: [Qemu-devel] [PATCH] ivshmem, qdev-monitor: fix order of qerror parameters
[not found] <1344944488-27075-1-git-send-email-agarcia@igalia.com>
@ 2012-08-14 13:42 ` Luiz Capitulino
2012-08-14 14:15 ` Alberto Garcia
0 siblings, 1 reply; 3+ messages in thread
From: Luiz Capitulino @ 2012-08-14 13:42 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-trivial, qemu-devel
On Tue, 14 Aug 2012 14:41:28 +0300
Alberto Garcia <agarcia@igalia.com> wrote:
> Now that the QERR_ macros no longer contain a json dictionary,
> the order of some parameters needs to be fixed for them to appear
> correctly.
> ---
> hw/ivshmem.c | 3 ++-
> hw/qdev-monitor.c | 2 +-
> 2 ficheiros modificados, 3 adições(+), 2 eliminados(-)
>
> diff --git a/hw/ivshmem.c b/hw/ivshmem.c
> index 0c58161..b4d65a6 100644
> --- a/hw/ivshmem.c
> +++ b/hw/ivshmem.c
> @@ -677,7 +677,8 @@ static int pci_ivshmem_init(PCIDevice *dev)
> }
>
> if (s->role_val == IVSHMEM_PEER) {
> - error_set(&s->migration_blocker, QERR_DEVICE_FEATURE_BLOCKS_MIGRATION, "ivshmem", "peer mode");
> + error_set(&s->migration_blocker, QERR_DEVICE_FEATURE_BLOCKS_MIGRATION,
> + "peer mode", "ivshmem");
Good catch, Alberto.
Here's the real problem. The original QERR_DEVICE_FEATURE_BLOCKS_MIGRATION was:
"{ 'class': 'DeviceFeatureBlocksMigration', 'data': { 'device': %s, 'feature': %s } }"
This is what was used in a error_set() call, so ordering was device name,
feature name.
However, the human error message format was:
"Migration is disabled when using feature '%(feature)' in device '%(device)'"
So, when constructing the error message, ordering was feature name, device name.
However, my script that moved all error messages from qerror.c to the QERR_
just did (as a final result):
error_set(errp,
"Migration is disabled when using feature '%s' in device '%s'",
device_name, feature_name);
Sh?t.
Now, I think that the best way to fix this is to change the error message
instead of fixing callers.
For example, we could have:
"Migration is disabled when device '%s' is using feature '%s'"
Additionally, we should check all old QERR_ macros to see if anyone else
swaps parameters like this and change them too.
I'll do it.
> migrate_add_blocker(s->migration_blocker);
> }
>
> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
> index b22a37a..018b386 100644
> --- a/hw/qdev-monitor.c
> +++ b/hw/qdev-monitor.c
> @@ -443,7 +443,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
> bus = qbus_find_recursive(sysbus_get_default(), NULL, k->bus_type);
> if (!bus) {
> qerror_report(QERR_NO_BUS_FOR_DEVICE,
> - driver, k->bus_type);
> + k->bus_type, driver);
> return NULL;
> }
> }
^ permalink raw reply [flat|nested] 3+ messages in thread