On Wed, Aug 29, 2012 at 11:15 PM, Amos Kong <kongjian...@gmail.com> wrote:
> On Thu, Feb 23, 2012 at 4:15 PM, Markus Armbruster <arm...@redhat.com> wrote:
>>
>> Anthony Liguori <anth...@codemonkey.ws> writes:
>>
>> > On 02/15/2012 07:33 AM, Markus Armbruster wrote:
>> >> Anthony Liguori<anth...@codemonkey.ws> writes:
>> >>
>> >>> On 02/14/2012 11:24 AM, Markus Armbruster wrote:
>> >>>> Markus Armbruster<arm...@redhat.com> writes:
>> >>>>
>> >>>>> Anthony Liguori<aligu...@us.ibm.com> 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().
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.
>
>
> From 4b8200ce662dd375819fd24cb932e70131ce0bd3 Mon Sep 17 00:00:00 2001
> From: Amos Kong <ak...@redhat.com>
> 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 <ak...@redhat.com>
> ---
> 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.
>>
>> [...]
>>
