[libvirt] [PATCH] virterror: Don't invoke error callback for ERR_OK
Since virDispatchError is now responsible for invoking the error callback, give it the same semantics as ReportError, which will skip VIR_ERR_OK (which is encountered when no error was raised). This fixes invoking the error callback after every non-erroring API call. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/util/virterror.c | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-) diff --git a/src/util/virterror.c b/src/util/virterror.c index e2128b9..78974ee 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -603,8 +603,12 @@ virDispatchError(virConnectPtr conn) if (!err) return; - /* Set a generic error message if none is already set */ + /* We never used to raise ERR_OK, so maintain existing behavior */ if (err->code == VIR_ERR_OK) + return; + + /* Set a generic error message if none is already set */ + if (!err->message) virErrorGenericFailure(err); /* Copy the global error to per-connection error if needed */ -- 1.6.5.2
On Tue, Jan 12, 2010 at 03:26:28PM -0500, Cole Robinson wrote:
Since virDispatchError is now responsible for invoking the error callback, give it the same semantics as ReportError, which will skip VIR_ERR_OK (which is encountered when no error was raised).
This fixes invoking the error callback after every non-erroring API call.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/util/virterror.c | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/src/util/virterror.c b/src/util/virterror.c index e2128b9..78974ee 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -603,8 +603,12 @@ virDispatchError(virConnectPtr conn) if (!err) return;
- /* Set a generic error message if none is already set */ + /* We never used to raise ERR_OK, so maintain existing behavior */ if (err->code == VIR_ERR_OK) + return; + + /* Set a generic error message if none is already set */ + if (!err->message) virErrorGenericFailure(err);
/* Copy the global error to per-connection error if needed */
We should only ever be invoking virDispatchError() in error paths, so if err->code == VIR_ERR_OK, this means we do need set a generic message because the earlier code indicated an error but forgot to report one. So I don't think this is correct. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On 01/12/2010 03:48 PM, Daniel P. Berrange wrote:
On Tue, Jan 12, 2010 at 03:26:28PM -0500, Cole Robinson wrote:
Since virDispatchError is now responsible for invoking the error callback, give it the same semantics as ReportError, which will skip VIR_ERR_OK (which is encountered when no error was raised).
This fixes invoking the error callback after every non-erroring API call.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/util/virterror.c | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/src/util/virterror.c b/src/util/virterror.c index e2128b9..78974ee 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -603,8 +603,12 @@ virDispatchError(virConnectPtr conn) if (!err) return;
- /* Set a generic error message if none is already set */ + /* We never used to raise ERR_OK, so maintain existing behavior */ if (err->code == VIR_ERR_OK) + return; + + /* Set a generic error message if none is already set */ + if (!err->message) virErrorGenericFailure(err);
/* Copy the global error to per-connection error if needed */
We should only ever be invoking virDispatchError() in error paths, so if err->code == VIR_ERR_OK, this means we do need set a generic message because the earlier code indicated an error but forgot to report one. So I don't think this is correct.
Ah, I think I wanted to check VIR_ERR_NONE here actually. virDispatchError is called regardless of whether an error is actually raised, so it may receive a zero'd out/empty virLastErrorObject, which is what I'm trying to avoid reporting. - Cole
On Tue, Jan 12, 2010 at 03:57:33PM -0500, Cole Robinson wrote:
On 01/12/2010 03:48 PM, Daniel P. Berrange wrote:
On Tue, Jan 12, 2010 at 03:26:28PM -0500, Cole Robinson wrote:
Since virDispatchError is now responsible for invoking the error callback, give it the same semantics as ReportError, which will skip VIR_ERR_OK (which is encountered when no error was raised).
This fixes invoking the error callback after every non-erroring API call.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/util/virterror.c | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/src/util/virterror.c b/src/util/virterror.c index e2128b9..78974ee 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -603,8 +603,12 @@ virDispatchError(virConnectPtr conn) if (!err) return;
- /* Set a generic error message if none is already set */ + /* We never used to raise ERR_OK, so maintain existing behavior */ if (err->code == VIR_ERR_OK) + return; + + /* Set a generic error message if none is already set */ + if (!err->message) virErrorGenericFailure(err);
/* Copy the global error to per-connection error if needed */
We should only ever be invoking virDispatchError() in error paths, so if err->code == VIR_ERR_OK, this means we do need set a generic message because the earlier code indicated an error but forgot to report one. So I don't think this is correct.
Ah, I think I wanted to check VIR_ERR_NONE here actually. virDispatchError is called regardless of whether an error is actually raised, so it may receive a zero'd out/empty virLastErrorObject, which is what I'm trying to avoid reporting.
I still don't think you are correct in that. If you run # grep --after 1 virDispatchError libvirt.c virDispatchError(NULL); return (-1); -- virDispatchError(net->conn); return -1; -- virDispatchError(NULL); return (-1); -- virDispatchError(pool->conn); return -1; Then all cases where virDispatchError() is called should be followed by the return of an error code, 99% of them are -1 or NULL. There are one or two where we use '0' for error as a special case. I don't see any places where virDispatchError() is called in a successful return path. So we should always be invoking the error callback, and ensuring an error is actually set before doing so. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On 01/13/2010 04:23 AM, Daniel P. Berrange wrote:
On Tue, Jan 12, 2010 at 03:57:33PM -0500, Cole Robinson wrote:
On 01/12/2010 03:48 PM, Daniel P. Berrange wrote:
On Tue, Jan 12, 2010 at 03:26:28PM -0500, Cole Robinson wrote:
Since virDispatchError is now responsible for invoking the error callback, give it the same semantics as ReportError, which will skip VIR_ERR_OK (which is encountered when no error was raised).
This fixes invoking the error callback after every non-erroring API call.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/util/virterror.c | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/src/util/virterror.c b/src/util/virterror.c index e2128b9..78974ee 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -603,8 +603,12 @@ virDispatchError(virConnectPtr conn) if (!err) return;
- /* Set a generic error message if none is already set */ + /* We never used to raise ERR_OK, so maintain existing behavior */ if (err->code == VIR_ERR_OK) + return; + + /* Set a generic error message if none is already set */ + if (!err->message) virErrorGenericFailure(err);
/* Copy the global error to per-connection error if needed */
We should only ever be invoking virDispatchError() in error paths, so if err->code == VIR_ERR_OK, this means we do need set a generic message because the earlier code indicated an error but forgot to report one. So I don't think this is correct.
Ah, I think I wanted to check VIR_ERR_NONE here actually. virDispatchError is called regardless of whether an error is actually raised, so it may receive a zero'd out/empty virLastErrorObject, which is what I'm trying to avoid reporting.
I still don't think you are correct in that. If you run
# grep --after 1 virDispatchError libvirt.c virDispatchError(NULL); return (-1); -- virDispatchError(net->conn); return -1; -- virDispatchError(NULL); return (-1); -- virDispatchError(pool->conn); return -1;
Then all cases where virDispatchError() is called should be followed by the return of an error code, 99% of them are -1 or NULL. There are one or two where we use '0' for error as a special case. I don't see any places where virDispatchError() is called in a successful return path. So we should always be invoking the error callback, and ensuring an error is actually set before doing so.
Whoops, sorry for the confusion. I'll investigate more. Thanks, Cole
On 01/13/2010 07:45 AM, Cole Robinson wrote:
On 01/13/2010 04:23 AM, Daniel P. Berrange wrote:
On Tue, Jan 12, 2010 at 03:57:33PM -0500, Cole Robinson wrote:
On 01/12/2010 03:48 PM, Daniel P. Berrange wrote:
On Tue, Jan 12, 2010 at 03:26:28PM -0500, Cole Robinson wrote:
Since virDispatchError is now responsible for invoking the error callback, give it the same semantics as ReportError, which will skip VIR_ERR_OK (which is encountered when no error was raised).
This fixes invoking the error callback after every non-erroring API call.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/util/virterror.c | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/src/util/virterror.c b/src/util/virterror.c index e2128b9..78974ee 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -603,8 +603,12 @@ virDispatchError(virConnectPtr conn) if (!err) return;
- /* Set a generic error message if none is already set */ + /* We never used to raise ERR_OK, so maintain existing behavior */ if (err->code == VIR_ERR_OK) + return; + + /* Set a generic error message if none is already set */ + if (!err->message) virErrorGenericFailure(err);
/* Copy the global error to per-connection error if needed */
We should only ever be invoking virDispatchError() in error paths, so if err->code == VIR_ERR_OK, this means we do need set a generic message because the earlier code indicated an error but forgot to report one. So I don't think this is correct.
Ah, I think I wanted to check VIR_ERR_NONE here actually. virDispatchError is called regardless of whether an error is actually raised, so it may receive a zero'd out/empty virLastErrorObject, which is what I'm trying to avoid reporting.
I still don't think you are correct in that. If you run
# grep --after 1 virDispatchError libvirt.c virDispatchError(NULL); return (-1); -- virDispatchError(net->conn); return -1; -- virDispatchError(NULL); return (-1); -- virDispatchError(pool->conn); return -1;
Then all cases where virDispatchError() is called should be followed by the return of an error code, 99% of them are -1 or NULL. There are one or two where we use '0' for error as a special case. I don't see any places where virDispatchError() is called in a successful return path. So we should always be invoking the error callback, and ensuring an error is actually set before doing so.
Whoops, sorry for the confusion. I'll investigate more.
A couple issues were conspiring to raise the 'Unknown error'. In the daemon we were blindly trying to unregister domain events via an API call, which would rightly error if not events had ever been registered. However, the implementation wasn't actually setting an error message. Patches coming shortly. Thanks, Cole
participants (2)
-
Cole Robinson -
Daniel P. Berrange