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

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

[Alberto Garcia]

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");
         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;
         }
     }
-- 
1.7.10.4

Re: [Qemu-trivial] [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-trivial] [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-trivial] [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>

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

[Stefan Hajnoczi]

On Tue, Aug 14, 2012 at 02:41:28PM +0300, Alberto Garcia 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(-)

Looks good but missing Signed-off-by:.

You can just reply with your Signed-off-by: line and I'll merge it, no
need to resend the patch.

For information on Signed-off-by:, please see "12) Sign your work":
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=Documentation/SubmittingPatches;h=689e2371095cc5dfea9927120009341f369159aa;hb=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#l297

Stefan

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

[Stefan Hajnoczi]

On Tue, Aug 14, 2012 at 02:41:28PM +0300, Alberto Garcia 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(-)

Thanks, applied to the trivial patches tree:
https://github.com/stefanha/qemu/commits/trivial-patches

Stefan