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

[Luiz Capitulino]

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.

> 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

 I agree, although there is going to be a huge effort in the conversion
work, as I think error handling in QEMU is not that good.