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

[Amos Kong]

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).
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().
Please help to review below RFC patch, thanks.


>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.
>
> [...]
>