Re: [Qemu-devel] [PATCH 09/19] sockets: Chardev open error reporting, sockets part
On Wed, 5 Sep 2012 10:19:22 +0800
Amos Kong kongjian...@gmail.com wrote:
On Thu, Aug 30, 2012 at 12:04 AM, Amos Kong kongjian...@gmail.com wrote:
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 Liguorianth...@codemonkey.ws writes:
On 02/14/2012 11:24 AM, Markus Armbruster wrote:
Markus Armbrusterarm...@redhat.com writes:
Anthony Liguorialigu...@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).
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
Re: [Qemu-devel] [PATCH 09/19] sockets: Chardev open error reporting, sockets part
On Thu, Aug 30, 2012 at 12:04 AM, Amos Kong kongjian...@gmail.com wrote:
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 Liguorianth...@codemonkey.ws writes:
On 02/14/2012 11:24 AM, Markus Armbruster wrote:
Markus Armbrusterarm...@redhat.com writes:
Anthony Liguorialigu...@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).
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
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:
Re: [Qemu-devel] [PATCH 09/19] sockets: Chardev open error reporting, sockets part
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 Liguorianth...@codemonkey.ws writes:
On 02/14/2012 11:24 AM, Markus Armbruster wrote:
Markus Armbrusterarm...@redhat.com writes:
Anthony Liguorialigu...@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().
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
Re: [Qemu-devel] [PATCH 09/19] sockets: Chardev open error reporting, sockets part
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 Liguorianth...@codemonkey.ws writes:
On 02/14/2012 11:24 AM, Markus Armbruster wrote:
Markus Armbrusterarm...@redhat.com writes:
Anthony Liguorialigu...@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
Re: [Qemu-devel] [PATCH 09/19] sockets: Chardev open error reporting, sockets part
Anthony Liguori anth...@codemonkey.ws writes:
On 02/15/2012 07:33 AM, Markus Armbruster wrote:
Anthony Liguorianth...@codemonkey.ws writes:
On 02/14/2012 11:24 AM, Markus Armbruster wrote:
Markus Armbrusterarm...@redhat.com writes:
Anthony Liguorialigu...@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'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.
[...]
Re: [Qemu-devel] [PATCH 09/19] sockets: Chardev open error reporting, sockets part
On 02/15/2012 07:33 AM, Markus Armbruster wrote:
Anthony Liguorianth...@codemonkey.ws writes:
On 02/14/2012 11:24 AM, Markus Armbruster wrote:
Markus Armbrusterarm...@redhat.com writes:
Anthony Liguorialigu...@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.
I'll focus on applying the patches you mentioned.
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 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.
We can't have async commands until qerror_report is removed from all existing
monitor commands. We can't plumb guest agent commands without async commands
not to mention that many commands are more naturally expressed as async commands
anyway.
Regards,
Anthony Liguori
Re: [Qemu-devel] [PATCH 09/19] sockets: Chardev open error reporting, sockets part
Anthony Liguori anth...@codemonkey.ws writes:
On 02/14/2012 11:24 AM, Markus Armbruster wrote:
Markus Armbrusterarm...@redhat.com writes:
Anthony Liguorialigu...@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.
Re: [Qemu-devel] [PATCH 09/19] sockets: Chardev open error reporting, sockets part
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. 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?
Re: [Qemu-devel] [PATCH 09/19] sockets: Chardev open error reporting, sockets part
On 02/14/2012 11:24 AM, Markus Armbruster wrote:
Markus Armbrusterarm...@redhat.com writes:
Anthony Liguorialigu...@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.
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.
Re: [Qemu-devel] [PATCH 09/19] sockets: Chardev open error reporting, sockets part
Anthony Liguori aligu...@us.ibm.com writes:
On 02/07/2012 08:09 AM, Markus Armbruster wrote:
Convert to error_report(). This adds error locations to the messages,
which is particularly important when the location is buried in a
configuration file. Moreover, we'll need this when we create a
monitor command to add character devices, so its errors actually
appear in the monitor, not stderr.
Also clean up the messages, and get rid of some that look like errors,
but aren't.
Improves user-hostile messages like this one for -chardev
socket,id=foo,host=blackfin,port=1,server
inet_listen_opts: bind(ipv4,192.168.2.9,1): Permission denied
chardev: opening backend socket failed
to
qemu-system-x86_64: -chardev socket,id=foo,host=blackfin,port=1,server:
Can't bind port blackfin:1: Permission denied
chardev: opening backend socket failed
and this one for -chardev udp,id=foo,localport=1,port=1
inet_dgram_opts: bind(ipv4,0.0.0.0,1): OK
inet_dgram_opts failed
chardev: opening backend udp failed
to
qemu-system-x86_64: -chardev udp,id=foo,localport=1,port=1: Can't bind
port :1: Permission denied
chardev: opening backend udp failed
You got to love the OK part.
The uninformative extra opening backend failed message will be
cleaned up shortly.
Signed-off-by: Markus Armbrusterarm...@redhat.com
---
qemu-char.c|1 -
qemu-sockets.c | 154
+--
2 files changed, 70 insertions(+), 85 deletions(-)
diff --git a/qemu-char.c b/qemu-char.c
index bb9e3f5..d591f70 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2101,7 +2101,6 @@ static CharDriverState *qemu_chr_open_udp(QemuOpts
*opts)
fd = inet_dgram_opts(opts);
if (fd 0) {
-fprintf(stderr, inet_dgram_opts failed\n);
goto return_err;
}
Let's add an Error ** argument here.
It's easy to bridge errors to error_report (qerror_report_err) so it's
conducive to incremental refactoring. Plus it starts getting us away
from terminal errors and into proper erroro propagation.
[...]
@@ -518,26 +499,31 @@ int unix_connect_opts(QemuOpts *opts)
int sock;
if (NULL == path) {
-fprintf(stderr, unix connect: no path specified\n);
+error_report(unix socket character device requires parameter
path);
return -1;
}
sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
if (sock 0) {
-perror(socket(unix));
-return -1;
+goto err;
}
memset(un, 0, sizeof(un));
un.sun_family = AF_UNIX;
snprintf(un.sun_path, sizeof(un.sun_path), %s, path);
if (connect(sock, (struct sockaddr*)un, sizeof(un)) 0) {
-fprintf(stderr, connect(unix:%s): %s\n, path, strerror(errno));
-close(sock);
-return -1;
+goto err;
}
return sock;
+
+err:
+error_report(Can't connect to socket %s: %s,
+ un.sun_path, strerror(errno));
+if (sock= 0) {
+close(sock);
+}
+return -1;
}
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.
You're imagining correctly. I've learned the hard way that replacing
nicely crafted error messages by error types frequently degrades the
error messages, which first makes me sad, and then makes me surf the web
rather than work.
You'd need somebody with a higher tolerance for bad error messages, or a
lower propensity to indulge in wasting time rather than deliver shoddy
work ;)
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.
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?
[Qemu-devel] [PATCH 09/19] sockets: Chardev open error reporting, sockets part
Convert to error_report(). This adds error locations to the messages,
which is particularly important when the location is buried in a
configuration file. Moreover, we'll need this when we create a
monitor command to add character devices, so its errors actually
appear in the monitor, not stderr.
Also clean up the messages, and get rid of some that look like errors,
but aren't.
Improves user-hostile messages like this one for -chardev
socket,id=foo,host=blackfin,port=1,server
inet_listen_opts: bind(ipv4,192.168.2.9,1): Permission denied
chardev: opening backend socket failed
to
qemu-system-x86_64: -chardev socket,id=foo,host=blackfin,port=1,server:
Can't bind port blackfin:1: Permission denied
chardev: opening backend socket failed
and this one for -chardev udp,id=foo,localport=1,port=1
inet_dgram_opts: bind(ipv4,0.0.0.0,1): OK
inet_dgram_opts failed
chardev: opening backend udp failed
to
qemu-system-x86_64: -chardev udp,id=foo,localport=1,port=1: Can't bind port
:1: Permission denied
chardev: opening backend udp failed
You got to love the OK part.
The uninformative extra opening backend failed message will be
cleaned up shortly.
Signed-off-by: Markus Armbruster arm...@redhat.com
---
qemu-char.c|1 -
qemu-sockets.c | 154 +--
2 files changed, 70 insertions(+), 85 deletions(-)
diff --git a/qemu-char.c b/qemu-char.c
index bb9e3f5..d591f70 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2101,7 +2101,6 @@ static CharDriverState *qemu_chr_open_udp(QemuOpts *opts)
fd = inet_dgram_opts(opts);
if (fd 0) {
-fprintf(stderr, inet_dgram_opts failed\n);
goto return_err;
}
diff --git a/qemu-sockets.c b/qemu-sockets.c
index 6bcb8e3..67e0559 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -20,7 +20,8 @@
#include unistd.h
#include qemu_socket.h
-#include qemu-common.h /* for qemu_isdigit */
+#include qemu-common.h
+#include qemu-error.h
#ifndef AI_ADDRCONFIG
# define AI_ADDRCONFIG 0
@@ -107,7 +108,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset)
char port[33];
char uaddr[INET6_ADDRSTRLEN+1];
char uport[33];
-int slisten, rc, to, port_min, port_max, p;
+int slisten, rc, to, port_min, port_max, p, sav_errno;
memset(ai,0, sizeof(ai));
ai.ai_flags = AI_PASSIVE | AI_ADDRCONFIG;
@@ -116,7 +117,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset)
if ((qemu_opt_get(opts, host) == NULL) ||
(qemu_opt_get(opts, port) == NULL)) {
-fprintf(stderr, %s: host and/or port not specified\n, __FUNCTION__);
+error_report(inet socket character device requires parameters host
and port);
return -1;
}
pstrcpy(port, sizeof(port), qemu_opt_get(opts, port));
@@ -133,8 +134,8 @@ int inet_listen_opts(QemuOpts *opts, int port_offset)
snprintf(port, sizeof(port), %d, atoi(port) + port_offset);
rc = getaddrinfo(strlen(addr) ? addr : NULL, port, ai, res);
if (rc != 0) {
-fprintf(stderr,getaddrinfo(%s,%s): %s\n, addr, port,
-gai_strerror(rc));
+error_report(Can't resolve %s:%s: %s,
+ addr, port, gai_strerror(rc));
return -1;
}
@@ -145,9 +146,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset)
NI_NUMERICHOST | NI_NUMERICSERV);
slisten = qemu_socket(e-ai_family, e-ai_socktype, e-ai_protocol);
if (slisten 0) {
-fprintf(stderr,%s: socket(%s): %s\n, __FUNCTION__,
-inet_strfamily(e-ai_family), strerror(errno));
-continue;
+continue; /* try next address */
}
setsockopt(slisten,SOL_SOCKET,SO_REUSEADDR,(void*)on,sizeof(on));
@@ -166,25 +165,33 @@ int inet_listen_opts(QemuOpts *opts, int port_offset)
if (bind(slisten, e-ai_addr, e-ai_addrlen) == 0) {
goto listen;
}
-if (p == port_max) {
-fprintf(stderr,%s: bind(%s,%s,%d): %s\n, __FUNCTION__,
-inet_strfamily(e-ai_family), uaddr, inet_getport(e),
-strerror(errno));
-}
}
+
+sav_errno = errno;
closesocket(slisten);
+errno = sav_errno;
+/* try next address */
+}
+
+/* no address worked, errno is from last failed socket() or bind() */
+if (to) {
+error_report(Can't bind any port %s:%s..%d: %s,
+ addr, port, to, strerror(errno));
+} else {
+error_report(Can't bind port %s:%s: %s,
+ addr, port, strerror(errno));
}
-fprintf(stderr, %s: FAILED\n, __FUNCTION__);
freeaddrinfo(res);
return -1;
listen:
if (listen(slisten,1) != 0) {
-perror(listen);
+error_report(Can't listen on %s:%d: %s, addr, p, strerror(errno));
closesocket(slisten);
Re: [Qemu-devel] [PATCH 09/19] sockets: Chardev open error reporting, sockets part
On 02/07/2012 08:09 AM, Markus Armbruster wrote:
Convert to error_report(). This adds error locations to the messages,
which is particularly important when the location is buried in a
configuration file. Moreover, we'll need this when we create a
monitor command to add character devices, so its errors actually
appear in the monitor, not stderr.
Also clean up the messages, and get rid of some that look like errors,
but aren't.
Improves user-hostile messages like this one for -chardev
socket,id=foo,host=blackfin,port=1,server
inet_listen_opts: bind(ipv4,192.168.2.9,1): Permission denied
chardev: opening backend socket failed
to
qemu-system-x86_64: -chardev socket,id=foo,host=blackfin,port=1,server:
Can't bind port blackfin:1: Permission denied
chardev: opening backend socket failed
and this one for -chardev udp,id=foo,localport=1,port=1
inet_dgram_opts: bind(ipv4,0.0.0.0,1): OK
inet_dgram_opts failed
chardev: opening backend udp failed
to
qemu-system-x86_64: -chardev udp,id=foo,localport=1,port=1: Can't bind
port :1: Permission denied
chardev: opening backend udp failed
You got to love the OK part.
The uninformative extra opening backend failed message will be
cleaned up shortly.
Signed-off-by: Markus Armbrusterarm...@redhat.com
---
qemu-char.c|1 -
qemu-sockets.c | 154 +--
2 files changed, 70 insertions(+), 85 deletions(-)
diff --git a/qemu-char.c b/qemu-char.c
index bb9e3f5..d591f70 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2101,7 +2101,6 @@ static CharDriverState *qemu_chr_open_udp(QemuOpts *opts)
fd = inet_dgram_opts(opts);
if (fd 0) {
-fprintf(stderr, inet_dgram_opts failed\n);
goto return_err;
}
Let's add an Error ** argument here.
It's easy to bridge errors to error_report (qerror_report_err) so it's conducive
to incremental refactoring. Plus it starts getting us away from terminal errors
and into proper erroro propagation.
diff --git a/qemu-sockets.c b/qemu-sockets.c
index 6bcb8e3..67e0559 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -20,7 +20,8 @@
#includeunistd.h
#include qemu_socket.h
-#include qemu-common.h /* for qemu_isdigit */
+#include qemu-common.h
+#include qemu-error.h
#ifndef AI_ADDRCONFIG
# define AI_ADDRCONFIG 0
@@ -107,7 +108,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset)
char port[33];
char uaddr[INET6_ADDRSTRLEN+1];
char uport[33];
-int slisten, rc, to, port_min, port_max, p;
+int slisten, rc, to, port_min, port_max, p, sav_errno;
memset(ai,0, sizeof(ai));
ai.ai_flags = AI_PASSIVE | AI_ADDRCONFIG;
@@ -116,7 +117,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset)
if ((qemu_opt_get(opts, host) == NULL) ||
(qemu_opt_get(opts, port) == NULL)) {
-fprintf(stderr, %s: host and/or port not specified\n, __FUNCTION__);
+error_report(inet socket character device requires parameters host and
port);
return -1;
}
pstrcpy(port, sizeof(port), qemu_opt_get(opts, port));
@@ -133,8 +134,8 @@ int inet_listen_opts(QemuOpts *opts, int port_offset)
snprintf(port, sizeof(port), %d, atoi(port) + port_offset);
rc = getaddrinfo(strlen(addr) ? addr : NULL, port,ai,res);
if (rc != 0) {
-fprintf(stderr,getaddrinfo(%s,%s): %s\n, addr, port,
-gai_strerror(rc));
+error_report(Can't resolve %s:%s: %s,
+ addr, port, gai_strerror(rc));
return -1;
}
@@ -145,9 +146,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset)
NI_NUMERICHOST | NI_NUMERICSERV);
slisten = qemu_socket(e-ai_family, e-ai_socktype, e-ai_protocol);
if (slisten 0) {
-fprintf(stderr,%s: socket(%s): %s\n, __FUNCTION__,
-inet_strfamily(e-ai_family), strerror(errno));
-continue;
+continue; /* try next address */
}
setsockopt(slisten,SOL_SOCKET,SO_REUSEADDR,(void*)on,sizeof(on));
@@ -166,25 +165,33 @@ int inet_listen_opts(QemuOpts *opts, int port_offset)
if (bind(slisten, e-ai_addr, e-ai_addrlen) == 0) {
goto listen;
}
-if (p == port_max) {
-fprintf(stderr,%s: bind(%s,%s,%d): %s\n, __FUNCTION__,
-inet_strfamily(e-ai_family), uaddr, inet_getport(e),
-strerror(errno));
-}
}
+
+sav_errno = errno;
closesocket(slisten);
+errno = sav_errno;
+/* try next address */
+}
+
+/* no address worked, errno is from last failed socket() or bind() */
+if (to) {
+error_report(Can't bind any port %s:%s..%d: %s,
+ addr, port, to, strerror(errno));
+} else {
+error_report(Can't bind port %s:%s: %s,
+
