From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MzrZE-0003Ep-2a for qemu-devel@nongnu.org; Mon, 19 Oct 2009 08:42:28 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MzrZ9-0003BN-1R for qemu-devel@nongnu.org; Mon, 19 Oct 2009 08:42:27 -0400 Received: from [199.232.76.173] (port=54933 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MzrZ8-0003BK-Rr for qemu-devel@nongnu.org; Mon, 19 Oct 2009 08:42:22 -0400 Received: from mx1.redhat.com ([209.132.183.28]:40307) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MzrZ8-0001Un-An for qemu-devel@nongnu.org; Mon, 19 Oct 2009 08:42:22 -0400 Date: Mon, 19 Oct 2009 13:42:13 +0100 From: "Daniel P. Berrange" Subject: Re: [Qemu-devel] Re: [PATCH 6/9] QError: Add qdev not found error Message-ID: <20091019124213.GJ27871@redhat.com> References: <20091015160839.7dbef5bf@doriath> <1255637617.29192.59.camel@slab.beaverton.ibm.com> <4AD78CCD.6030006@us.ibm.com> <1255641511.29192.68.camel@slab.beaverton.ibm.com> <4AD793C6.9060508@us.ibm.com> <1255646688.3954.45.camel@slab.beaverton.ibm.com> <4AD82972.7010200@redhat.com> <20091016100544.266442f5@doriath> <20091019102519.GB27871@redhat.com> <20091019102817.03a3c843@doriath> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20091019102817.03a3c843@doriath> Reply-To: "Daniel P. Berrange" List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: Paolo Bonzini , Anthony Liguori , kraxel@redhat.com, Hollis Blanchard , qemu-devel@nongnu.org On Mon, Oct 19, 2009 at 10:28:17AM -0200, Luiz Capitulino wrote: > On Mon, 19 Oct 2009 11:25:19 +0100 > "Daniel P. Berrange" wrote: > > > On Fri, Oct 16, 2009 at 10:05:44AM -0300, Luiz Capitulino wrote: > > > On Fri, 16 Oct 2009 10:06:10 +0200 > > > Paolo Bonzini wrote: > > > > > > > On 10/16/2009 12:44 AM, Hollis Blanchard wrote: > > > > > How about this (basically what Paolo suggested): > > > > > > > > > > { "error": { "code": 12, > > > > > "desc": "device %{bus}:%{address} already open", > > > > > "data": { "bus": 0, "address": 12 } } } > > > > > > > > > > 'desc'*may* be used by the client, or may be replaced with a localized > > > > > version. > > > > > > > > I would say that desc need not go on the wire too. The client might not > > > > even want to show the same string to the user, for example they may want > > > > to say "mouse already" open. > > > > > > > > The "device %{bus}:%{address} already open" would be strictly inside > > > > QEMU, for consumption of the monitor interface. Of course since the > > > > server is in QEMU too it makes sense to consolidate it in the same > > > > struct, but this does not mean that everything in the struct needs to go > > > > on the wire. > > > > > > This is what my current proposal does, "desc" goes on the wire > > > because it's a simple description but messages for users consumption > > > are printed by .user_print. > > > > > > I think we could make this very simple if we use a solution along > > > the lines suggested by Hollis and do _not_ allow variable information > > > (ie. 'handler data') to go on the wire. > > > > > > I mean, we could let current errors as they are but add error codes > > > and new error descriptions to be used by the protocol only. > > > > > > For example, a call to: > > > > > > monitor_printf(mon, "husb: host usb device %d.%d is already open\n", > > > bus_num, addr); > > > > > > Would become: > > > > > > qemu_error_structed(404, "husb: host usb device %d.%d is already open\n", > > > bus_num, addr); > > > > > > When in user protocol the message is printed normally, when in protocol > > > mode the error code is used to index the error table and on the wire > > > we emit: > > > > > > { "error": { "code": 404, "desc": "device already open" } } > > > > > > Now I need to know if it's ok to have such simple error information > > > on the wire. > > > > In so much as its providing an error code + message I think that is > > sufficient, but I think we should take care to ensure that the error > > description contains enough information to allow useful troubleshooting. > > As such doing a simple index lookup from error code -> message is not > > sufficient, because it would be throwing away potentially important > > error data. > > Yes, it's going to throw away important info. We could have a big enough > table with all possible errors, but this seems insane at first. > > Another solution could be to change the semantics of the error data, > instead of passing to it information which is already there we could pass > additional info which is not part of the original message. > > On the other hand, I think this will make the usage of qemu_error_structed() > a bit confusing. Why not include all of it, the error code, the generic string for the error code, and then the formatted specific error string the monitor currently has. eg { "error": { "code": 404, "desc": "device already open", "detail": "husb: host usb device 001.003 is already open" } } Since the latter error messages all already exist, it should make it easier to adapt, and allow clients flexibility in how they report/handle the errors whether to show the detail error to users, or just the generic msg Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|