From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MyUUy-0002AA-Vo for qemu-devel@nongnu.org; Thu, 15 Oct 2009 13:52:25 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MyUUu-00026l-3D for qemu-devel@nongnu.org; Thu, 15 Oct 2009 13:52:24 -0400 Received: from [199.232.76.173] (port=46896 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MyUUt-00026I-R3 for qemu-devel@nongnu.org; Thu, 15 Oct 2009 13:52:19 -0400 Received: from mx1.redhat.com ([209.132.183.28]:65389) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MyUUr-0008FH-HS for qemu-devel@nongnu.org; Thu, 15 Oct 2009 13:52:18 -0400 Date: Thu, 15 Oct 2009 14:52:08 -0300 From: Luiz Capitulino Subject: Re: [Qemu-devel] [PATCH 6/9] QError: Add qdev not found error Message-ID: <20091015145208.1d871f09@doriath> In-Reply-To: <1255626960.29192.7.camel@slab.beaverton.ibm.com> References: <1255453026-18637-1-git-send-email-lcapitulino@redhat.com> <1255453026-18637-7-git-send-email-lcapitulino@redhat.com> <1255561330.29192.2.camel@slab.beaverton.ibm.com> <20091015103405.591e2f3b@doriath> <1255626960.29192.7.camel@slab.beaverton.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Hollis Blanchard Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org, kraxel@redhat.com On Thu, 15 Oct 2009 10:16:00 -0700 Hollis Blanchard wrote: > On Thu, 2009-10-15 at 10:34 -0300, Luiz Capitulino wrote: > > On Wed, 14 Oct 2009 16:02:10 -0700 > > Hollis Blanchard wrote: > > > > > On Tue, 2009-10-13 at 13:57 -0300, Luiz Capitulino wrote: > > > > Signed-off-by: Luiz Capitulino > > > > --- > > > > qerror.c | 12 ++++++++++++ > > > > qerror.h | 1 + > > > > 2 files changed, 13 insertions(+), 0 deletions(-) > > > > > > > > diff --git a/qerror.c b/qerror.c > > > > index bbea770..88a8208 100644 > > > > --- a/qerror.c > > > > +++ b/qerror.c > > > > @@ -24,11 +24,23 @@ static const QType qerror_type = { > > > > .destroy = qerror_destroy_obj, > > > > }; > > > > > > > > +static void qemu_err_qdev_nodev(const QError *qerror) > > > > +{ > > > > + QDict *qdict = qobject_to_qdict(qerror->data); > > > > + qemu_error("Device \"%s\" not found. Try -device '?' for a list.\n", > > > > + qdict_get_str(qdict, "name")); > > > > +} > > > > + > > > > static QErrorTable qerror_table[] = { > > > > { > > > > .code = QERR_UNKNOWN, > > > > .desc = "unknown error", > > > > }, > > > > + { > > > > + .code = QERR_QDEV_NFOUND, > > > > + .desc = "device not found", > > > > + .user_print = qemu_err_qdev_nodev, > > > > + }, > > > > }; > > > > > > > > /** > > > > diff --git a/qerror.h b/qerror.h > > > > index ed25ef1..820f25e 100644 > > > > --- a/qerror.h > > > > +++ b/qerror.h > > > > @@ -21,6 +21,7 @@ > > > > */ > > > > typedef enum QErrorCode { > > > > QERR_UNKNOWN, > > > > + QERR_QDEV_NFOUND, > > > > QERR_MAX, > > > > } QErrorCode; > > > > > > I'm not really seeing the point: what is gained by moving the error text > > > from the original site into this function-inside-a-structure? > > > > Compatibility and a way of having pretty printing functions w/o > > breaking the machine protocol. > > Huh? Compatibility with what? With existing errors. I'm avoiding changing them because existing applications which parse QEMU output may rely on them. On the other hand, I'm not sure if this is a hard requirement. > I don't understand this "pretty printing" comment either. The structured error output can sometimes be too rigid for humans, this qdev error is an example. When we fail we pass the 'Try -device' hint, which doesn't make sense for the protocol. Also, it's not uncommon to have error strings like this: monitor_printf(mon, "husb: host usb device %d.%d is already open\n", bus_num, addr); Which is what I call 'pretty printing'. > You could easily have > qemu_error(code, "device not found"); This doesn't solve the problems I mentioned above, besides I don't see why you need to specify both, the error code and the description, they describe the same thing. Also, they must not change after the protocol gets into production, having them defined in the same place will help.