Re: [Qemu-devel] Re: [PATCH 6/9] QError: Add qdev not found error

[Daniel P. Berrange]

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" <address@hidden> 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 <address@hidden> 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 :|