From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:39265) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T1HNs-0002xv-3t for qemu-devel@nongnu.org; Tue, 14 Aug 2012 09:42:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T1HNq-0003Vd-Vz for qemu-devel@nongnu.org; Tue, 14 Aug 2012 09:42:12 -0400 Date: Tue, 14 Aug 2012 10:42:30 -0300 From: Luiz Capitulino Message-ID: <20120814104230.64aed4e3@doriath.home> In-Reply-To: <1344944488-27075-1-git-send-email-agarcia@igalia.com> References: <1344944488-27075-1-git-send-email-agarcia@igalia.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] ivshmem, qdev-monitor: fix order of qerror parameters List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alberto Garcia Cc: qemu-trivial@nongnu.org, qemu-devel On Tue, 14 Aug 2012 14:41:28 +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=E7=F5es(+), 2 eliminados(-) >=20 > 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) > } > =20 > if (s->role_val =3D=3D IVSHMEM_PEER) { > - error_set(&s->migration_blocker, QERR_DEVICE_FEATURE_BLOCKS_MIGR= ATION, "ivshmem", "peer mode"); > + error_set(&s->migration_blocker, QERR_DEVICE_FEATURE_BLOCKS_MIGR= ATION, > + "peer mode", "ivshmem"); Good catch, Alberto. Here's the real problem. The original QERR_DEVICE_FEATURE_BLOCKS_MIGRATION = was: "{ 'class': 'DeviceFeatureBlocksMigration', 'data': { 'device': %s, 'featu= re': %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); > } > =20 > 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 =3D qbus_find_recursive(sysbus_get_default(), NULL, k->bus_t= ype); > if (!bus) { > qerror_report(QERR_NO_BUS_FOR_DEVICE, > - driver, k->bus_type); > + k->bus_type, driver); > return NULL; > } > }