Re: [Qemu-devel] [PATCH 09/19] sockets: Chardev open error reporting, sockets part

[Luiz Capitulino]

On Wed, 5 Sep 2012 10:19:22 +0800
Amos Kong <address@hidden> wrote:

> On Thu, Aug 30, 2012 at 12:04 AM, Amos Kong <address@hidden> wrote:
> 
> > On Wed, Aug 29, 2012 at 11:15 PM, Amos Kong <address@hidden> wrote:
> > > On Thu, Feb 23, 2012 at 4:15 PM, Markus Armbruster <address@hidden>
> > wrote:
> > >>
> > >> Anthony Liguori <address@hidden> writes:
> > >>
> > >> > On 02/15/2012 07:33 AM, Markus Armbruster wrote:
> > >> >> Anthony Liguori<address@hidden>  writes:
> > >> >>
> > >> >>> On 02/14/2012 11:24 AM, Markus Armbruster wrote:
> > >> >>>> Markus Armbruster<address@hidden>   writes:
> > >> >>>>
> > >> >>>>> Anthony Liguori<address@hidden>   writes:
> > >> >>>> [Anthony asking for error_set() instead of error_report()...]
> > >> >>>>>> Basically, same thing here and the remaining functions.  Let's
> > not
> > >> >>>>>> introduce additional uses of error_report().
> > >> >>>>>>
> > >> >>>>>> That said, I imagine you don't want to introduce a bunch of error
> > >> >>>>>> types for these different things and that's probably not
> > productive
> > >> >>>>>> anyway.
> > >> >>>> [...]
> > >> >>>>>> So let's compromise and introduce a generic QERR_INTERNAL_ERROR
> > that
> > >> >>>>>> takes a single human readable string as an argument.  We can
> > have a
> > >> >>>>>> wrapper for it that also records location information in the
> > error
> > >> >>>>>> object.
> > >
> > >
> > >
> > >
> > >
> > >>
> > >> >>>>> This series goes from stderr to error_report().  That's a
> > relatively
> > >> >>>>> simple step, which makes it relatively easy to review.  I'm afraid
> > >> >>>>> moving all the way to error.h in one step wouldn't be as easy.
> >  Kevin
> > >> >>>>> suggests to do it in a follow-up series, and I agree.
> > >> >>>
> > >> >>> The trouble I have is not about doing things incrementally, but
> > rather
> > >> >>> touching a lot of code incrementally.  Most of the code you touch
> > >> >>> could be done incrementally with error_set().
> > >> >>>
> > >> >>> For instance, you could touch inet_listen_opts() and just add an
> > Error
> > >> >>> ** as the last argument.  You can change all callers to simply do:
> > >> >>>
> > >> >>> Error *err = NULL;
> > >> >>>
> > >> >>> ...
> > >> >>> inet_listen_opts(...,&err);
> > >> >>> if (err) {
> > >> >>>     error_report_err(err);
> > >> >>>     return -1;
> > >> >>> }
> > >> >>>
> > >> >>> And it's not really all that different from the series as it stands
> > >> >>> today.  I agree that aggressively refactoring error propagation is
> > >> >>> probably not necessary as a first step, but if we're going to touch
> > a
> > >> >>> lot of code, we should do it in a way that we don't have to
> > >> >>> immediately touch it again next.
> > >> >>
> > >> >> Well, the series adds 47 calls of error_report() to five files out of
> > >> >> 1850.
> > >> >>
> > >> >>>>> Can you point to an existing conversion from error_report() to
> > error.h,
> > >> >>>>> to give us an idea how it's supposed to be done?
> > >> >>>>
> > >> >>>> Ping?
> > >> >>>
> > >> >>> Sorry, I mentally responded bug neglected to actually respond.
> > >> >>>
> > >> >>> All of the QMP work that Luiz is doing effectively does this so
> > there
> > >> >>> are ample examples right now.  The change command is probably a good
> > >> >>> place to start.
> > >> >>
> > >> >> Thanks.
> > >> >>
> > >> >> Unfortunately, I'm out of time on this one, so if you're unwilling to
> > >> >> accept this admittedly incremental improvement without substantial
> > >> >> rework, I'll have to let it rot in peace.
> > >> >>
> > >> >> We might want a QMP commands to add character devices some day.
> >  Perhaps
> > >> >> the person doing it will still be able to find these patches, and get
> > >> >> some use out of them.
> > >> >>
> > >> >> Patches 01-08,14 don't add error_report() calls.  What about
> > committing
> > >> >> them?  The commit messages would need to be reworded not to promise
> > >> >> goodies from the other patches, though.
> > >> >
> > >> > I'm sorry to hear that you can't continue working on this.
> > >>
> > >> Can't be helped.
> > >
> > >
> > >
> > > I want to continue working on this work (patch 9~13,15~19).
> >
> 
> 
> FYI, URL of the old thread:
> http://lists.nongnu.org/archive/html/qemu-devel/2012-02/msg00714.html
> 
> 
> 
> > > Makus used error_report() to output the rich/meaningful error message
> > > to monitor,
> > > but Anthony prefers to use error_set(), right?
> > >
> > > I would like to introduce a generic QERR_INTERNAL_ERROR as Anthony
> > > said to replace error_report().
> >
> >
> > error_report() can be used many times/places, we might see many error
> > in monitor.
> > but error_set() can only set one kind of error when internal error
> > occurs, and only
> > one error will be printed into monitor.
> >
> > output final/important error by error_set()/error_report_err(), and
> > output other error to stdio?
> >
> > ---
> > There are some 'GENERIC ERROR CLASS' in qerror.h, which are not used
> > very frequently, we can convert them to 'QERR_INTERNAL_ERROR'.
> >
> > Or convert all 'GENERIC ERROR CLASS' to  'QERR_INTERNAL_ERROR',
> > and use a single human readable string?
> >
> > > Please help to review below RFC patch, thanks.
> >
> >
> Anthony, Luiz, other
> 
> any comments?
> 
> archive of this patch:
> http://lists.nongnu.org/archive/html/qemu-devel/2012-08/msg04927.html

That thread is too old, that work should be re-done on top of the new
error format.

However, we have bug 603266 for that and it has been assigned to me. And
I have started working on it already...

> 
> 
> > From 4b8200ce662dd375819fd24cb932e70131ce0bd3 Mon Sep 17 00:00:00 2001
> > > From: Amos Kong <address@hidden>
> > > Date: Wed, 29 Aug 2012 22:52:48 +0800
> > > Subject: [PATCH RFC] qerror: introduce QERR_INTERNAL_ERROR
> > >
> > > Current qerror reporting infrastructure isn't agile,
> > > we have to add a new Class for a new error.
> > >
> > > This patch introduced a generic QERR_INTERNAL_ERROR
> > > that takes a single human readable string as an argument.
> > >
> > > This patch is a RFC, so I only changed some code of
> > > inet_connect() as an example.
> > >
> > > hmp:
> > > (qemu) migrate -d tcp:noname:4446
> > > migrate: Can't resolve noname:4446: Name or service not known
> > >
> > > qmp:
> > > { "execute": "migrate", "arguments": { "uri": "tcp:noname:4446" } }
> > > {"error": {"class": "GenericError", "desc": "Can't resolve noname:4446:
> > > Name or service not known"}}
> > >
> > > Signed-off-by: Amos Kong <address@hidden>
> > > ---
> > >  qemu-sockets.c |    9 +++++----
> > >  qerror.h       |    3 +++
> > >  2 files changed, 8 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/qemu-sockets.c b/qemu-sockets.c
> > > index 361d890..e528288 100644
> > > --- a/qemu-sockets.c
> > > +++ b/qemu-sockets.c
> > > @@ -244,9 +244,9 @@ int inet_connect_opts(QemuOpts *opts, bool
> > > *in_progress, Error **errp)
> > >
> > >      /* lookup */
> > >      if (0 != (rc = getaddrinfo(addr, port, &ai, &res))) {
> > > -        fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port,
> > > -                gai_strerror(rc));
> > > -        error_set(errp, QERR_SOCKET_CREATE_FAILED);
> > > +        char err[50];
> > > +        sprintf(err, "Can't resolve %s:%s: %s", addr, port,
> > gai_strerror(rc));
> > > +        error_set(errp, QERR_INTERNAL_ERROR, err);
> > >         return -1;
> > >      }
> > >
> > > @@ -505,7 +505,8 @@ int inet_connect(const char *str, bool block, bool
> > > *in_progress, Error **errp)
> > >          }
> > >          sock = inet_connect_opts(opts, in_progress, errp);
> > >      } else {
> > > -        error_set(errp, QERR_SOCKET_CREATE_FAILED);
> > > +        error_set(errp, QERR_INTERNAL_ERROR,
> > > +                  "inet_connect: failed to parse address");
> > >      }
> > >      qemu_opts_del(opts);
> > >      return sock;
> > > diff --git a/qerror.h b/qerror.h
> > > index d0a76a4..97bf5e0 100644
> > > --- a/qerror.h
> > > +++ b/qerror.h
> > > @@ -246,4 +246,7 @@ void assert_no_error(Error *err);
> > >  #define QERR_SOCKET_CREATE_FAILED \
> > >      ERROR_CLASS_GENERIC_ERROR, "Failed to create socket"
> > >
> > > +#define QERR_INTERNAL_ERROR \
> > > +    ERROR_CLASS_GENERIC_ERROR, "%s"
> > > +
> > >  #endif /* QERROR_H */
> > > --
> > > 1.7.1
> > >
> > >
> > >
> > >>
> > >>
> > >> > I'll focus on applying the patches you mentioned.
> > >>
> > >> Suggest to reword the commit messages not to promise the parts you don't
> > >> apply.
> > >>
> > >> > While I admit that it seems counter intuitive to not want to improve
> > >> > error messages (and I fully admit, that this is an improvement), I'm
> > >> > more concerned that this digs us deeper into the
> > >> > qerror_report/error_report hole that we're trying to dig our way out
> > >> > of.
> > >> >
> > >> > If you want to add chardevs dynamically, then I assume your next patch
> > >>
> > >> Not a priority at this time, I'm afraid.  If it becomes one, I might be
> > >> able to work on it.
> > >>
> > >> > would be switching error_report to qerror_report such that the errors
> > >> > appeared in the monitor.  But this is wrong.  New QMP functions are
> > >> > not allowed to use qerror_report anymore.  So all of this code would
> > >> > need to get changed again anyway.
> > >>
> > >> No, the next step for errors would be error_report() -> error_set(),
> > >> precisely because qerror_report() can't be used anymore.
> > >>
> > >> Yes, that means the five files that report chardev open errors will need
> > >> to be touched again.  But that'll be a bog-standard error_report() ->
> > >> error_set() conversion.  Easier to code, test and review than combined
> > >> "track down all the error paths that fail to report errors, or report
> > >> non-sensical errors" + "convert from fprintf() to error_set() in one
> > >> go".
> > >>
> > >> In my opinion, the "have to touch five files again" developer burden
> > >> compares quite favorably with "have to check all the error paths again"
> > >> developer burden.  And in any case it's dwarved by the "have to use a
> > >> debugger to find out what's wrong" user burden.
> > >>
> > >> [...]
> > >>
> >