Re: [Qemu-devel] [PATCH] ivshmem, qdev-monitor: fix order of qerror parameters

Re: [Qemu-devel] [PATCH] ivshmem, qdev-monitor: fix order of qerror parameters

[Luiz Capitulino]

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;
>          }
>      }

Re: [Qemu-devel] [PATCH] ivshmem, qdev-monitor: fix order of qerror parameters

[Alberto Garcia]

On Tue, Aug 14, 2012 at 10:42:30AM -0300, Luiz Capitulino wrote:

> Additionally, we should check all old QERR_ macros to see if anyone
> else swaps parameters like this and change them too.

Yes, I also noticed the problem and checked all QERR_ macros, I only
found the two that I corrected in the patch, but double checking the
list won't hurt I guess :)

Berto

Re: [Qemu-devel] [PATCH] ivshmem, qdev-monitor: fix order of qerror parameters

[Luiz Capitulino]

On Tue, 14 Aug 2012 16:15:37 +0200
Alberto Garcia <agarcia@igalia.com> wrote:

> On Tue, Aug 14, 2012 at 10:42:30AM -0300, Luiz Capitulino wrote:
> 
> > Additionally, we should check all old QERR_ macros to see if anyone
> > else swaps parameters like this and change them too.
> 
> Yes, I also noticed the problem and checked all QERR_ macros, I only
> found the two that I corrected in the patch, but double checking the
> list won't hurt I guess :)

Yes, you're right. I've just double checked it and these are the only
two cases and both have only one single user.

This patch is fine then:

 Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>