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

[Daniel P. Berrange]

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.

Consider a user files a bug report attaching client app logs which
show the monitor command issued and the it error returned. Ask yourself,
is that enough information to locate where in the code the error came
from & can you make a reasonable attempt at identifying a root cause. 

In your example above, the usb_add command already included the bus_num
+ addr, so there'd be no need to also include that data in the reply.
But consider when QEMU actually tries to open the host USB device, there
are easily 10-20 different things that could go wrong, and many even
simple system calls like open() could generate many different errors.
It is important that this fine detail be reported back to the client
app for developers to have a good attempt at troubleshooting

Regards,
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 :|