Re: [Qemu-devel] [PATCH 7/9] qdev: Use QError for not found error

["Daniel P. Berrange" <berrange@redhat.com>]

On Wed, Oct 14, 2009 at 12:34:21AM +0200, Markus Armbruster wrote:
> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  hw/qdev.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/qdev.c b/hw/qdev.c
> > index 906e897..3ce48f7 100644
> > --- a/hw/qdev.c
> > +++ b/hw/qdev.c
> > @@ -28,6 +28,7 @@
> >  #include "net.h"
> >  #include "qdev.h"
> >  #include "sysemu.h"
> > +#include "qerror.h"
> >  #include "monitor.h"
> >  
> >  static int qdev_hotplug = 0;
> > @@ -176,8 +177,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
> >      /* find driver */
> >      info = qdev_find_info(NULL, driver);
> >      if (!info) {
> > -        qemu_error("Device \"%s\" not found.  Try -device '?' for a list.\n",
> > -                   driver);
> > +        qemu_error_structed(QERR_QDEV_NFOUND, "{ s: s }", "name", driver);
> >          return NULL;
> >      }
> >      if (info->no_user) {

> Now let's look at errors from the client's perspective.
> 
> Clients need to classify errors to figure out how to respond to them.
> Something like client error (i.e. your command was stupid, and trying
> again will be just as stupid), permanent server error (it wasn't stupid,
> but I can't do it for you), transient server error (I couldn't do it for
> you, but trying again later could help).
> 
> Some classical protocols (HTTP, FTP) provide error class (they cleverly
> encode it into the error code).  This gives clients a chance to sanely
> handle errors they don't know.
> 
> Even with error class figured out, clients may still be interested in
> the exact error code, at least in some cases.
> 
> They may also be interested in a human readable description of the
> error, to present to their human users.  Some classical protocols
> provide that in addition to an error code.  This gives clients a chance
> to sanely report errors they don't know to human users.
> 
> I suspect that the additional error data is the error's least
> interesting part most of the time.  When we get QERR_QDEV_NFOUND, I
> figure it's usually clear what device we were trying to find.
> 
> But these are just my educated guesses.  I'd love to hear what folks
> involved with actual clients have to say.  Anyone from libvirt?

I think just returning error codes to the client is far too little
information. I don't think we need the fully normalized structure
that Luiz originally proposed with bus/dev addresses split out, but
we certainly need to include a string description giving as much
detail as possible.  If attaching a host USB device failed, I don't
want a single error code  QERR_OPEN_FAILED, nor a generic message 
like 'could not open device', i want the exact details, eg

  'could not open device: permission denied' 
  'could not open device: no such file or directory' 
  'could not open device: device or resource busy' 

The localization issue is somewhat of a red herring though. This string
description is not something that clients would ultimately expose to the
user. The client apps would likely present a more generic error message
to the user, based off the error code. The error description received
from QEMU will instead be written out to the logs as a record for later
troubleshooting. eg if the user files a bug report, it will include the
full error details, so libvirt/qemu maintainers have a better chance of
figuring out what went wrong.

So ultimately these error messages are for developers, not users benefit.

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