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

[Luiz Capitulino]

On Thu, 15 Oct 2009 10:16:00 -0700
Hollis Blanchard <address@hidden> wrote:

> On Thu, 2009-10-15 at 10:34 -0300, Luiz Capitulino wrote:
> > On Wed, 14 Oct 2009 16:02:10 -0700
> > Hollis Blanchard <address@hidden> wrote:
> > 
> > > On Tue, 2009-10-13 at 13:57 -0300, Luiz Capitulino wrote:
> > > > Signed-off-by: Luiz Capitulino <address@hidden>
> > > > ---
> > > >  qerror.c |   12 ++++++++++++
> > > >  qerror.h |    1 +
> > > >  2 files changed, 13 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/qerror.c b/qerror.c
> > > > index bbea770..88a8208 100644
> > > > --- a/qerror.c
> > > > +++ b/qerror.c
> > > > @@ -24,11 +24,23 @@ static const QType qerror_type = {
> > > >      .destroy = qerror_destroy_obj,
> > > >  };
> > > > 
> > > > +static void qemu_err_qdev_nodev(const QError *qerror)
> > > > +{
> > > > +    QDict *qdict = qobject_to_qdict(qerror->data);
> > > > +    qemu_error("Device \"%s\" not found.  Try -device '?' for a 
> > > > list.\n",
> > > > +               qdict_get_str(qdict, "name"));
> > > > +}
> > > > +
> > > >  static QErrorTable qerror_table[] = {
> > > >      {
> > > >          .code   = QERR_UNKNOWN,
> > > >          .desc   = "unknown error",
> > > >      },
> > > > +    {
> > > > +        .code   = QERR_QDEV_NFOUND,
> > > > +        .desc   = "device not found",
> > > > +        .user_print = qemu_err_qdev_nodev,
> > > > +    },
> > > >  };
> > > > 
> > > >  /**
> > > > diff --git a/qerror.h b/qerror.h
> > > > index ed25ef1..820f25e 100644
> > > > --- a/qerror.h
> > > > +++ b/qerror.h
> > > > @@ -21,6 +21,7 @@
> > > >   */
> > > >  typedef enum QErrorCode {
> > > >      QERR_UNKNOWN,
> > > > +    QERR_QDEV_NFOUND,
> > > >      QERR_MAX,
> > > >  } QErrorCode;
> > > 
> > > I'm not really seeing the point: what is gained by moving the error text
> > > from the original site into this function-inside-a-structure?
> > 
> >  Compatibility and a way of having pretty printing functions w/o
> > breaking the machine protocol.
> 
> Huh? Compatibility with what?

 With existing errors. I'm avoiding changing them because existing
applications which parse QEMU output may rely on them.

 On the other hand, I'm not sure if this is a hard requirement.

> I don't understand this "pretty printing" comment either.

 The structured error output can sometimes be too rigid for humans,
this qdev error is an example. When we fail we pass the 'Try -device'
hint, which doesn't make sense for the protocol.

 Also, it's not uncommon to have error strings like this:

monitor_printf(mon, "husb: host usb device %d.%d is already open\n",
                      bus_num, addr);

 Which is what I call 'pretty printing'.
 
> You could easily have
>       qemu_error(code, "device not found");

 This doesn't solve the problems I mentioned above, besides I don't
see why you need to specify both, the error code and the description,
they describe the same thing.

 Also, they must not change after the protocol gets into production,
having them defined in the same place will help.