[libvirt] [PATCH] don't test "res == NULL" after we've already dereferenced it
As the log says, once we've dereferenced it, there's no point in comparing to NULL.
From 463eaf1027a154e71839a67eca85b3ada8b817ff Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Wed, 6 Jan 2010 12:45:07 +0100 Subject: [PATCH] don't test "res == NULL" after we've already dereferenced it
* src/xen/proxy_internal.c (xenProxyCommand): It is known to be non-NULL at that point, so remove the "ret == NULL" guard, and instead add an assert(ret != NULL), in case future code changes cause the condition to becomes false. Include <assert.h>. --- src/xen/proxy_internal.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/xen/proxy_internal.c b/src/xen/proxy_internal.c index ec4522b..42b6e91 100644 --- a/src/xen/proxy_internal.c +++ b/src/xen/proxy_internal.c @@ -1,7 +1,7 @@ /* * proxy_client.c: client side of the communication with the libvirt proxy. * - * Copyright (C) 2006, 2008, 2009 Red Hat, Inc. + * Copyright (C) 2006, 2008, 2009, 2010 Red Hat, Inc. * * See COPYING.LIB for the License of this software * @@ -11,6 +11,7 @@ #include <config.h> #include <stdio.h> +#include <assert.h> #include <stdlib.h> #include <unistd.h> #include <errno.h> @@ -444,7 +445,8 @@ retry: /* * do more checks on the incoming packet. */ - if ((res == NULL) || (res->version != PROXY_PROTO_VERSION) || + assert (res != NULL); + if ((res->version != PROXY_PROTO_VERSION) || (res->len < sizeof(virProxyPacket))) { virProxyError(conn, VIR_ERR_INTERNAL_ERROR, "%s", _("Communication error with proxy: malformed packet\n")); -- 1.6.6.387.g2649b1
On Wed, Jan 06, 2010 at 12:46:11PM +0100, Jim Meyering wrote:
As the log says, once we've dereferenced it, there's no point in comparing to NULL.
From 463eaf1027a154e71839a67eca85b3ada8b817ff Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Wed, 6 Jan 2010 12:45:07 +0100 Subject: [PATCH] don't test "res == NULL" after we've already dereferenced it
* src/xen/proxy_internal.c (xenProxyCommand): It is known to be non-NULL at that point, so remove the "ret == NULL" guard, and instead add an assert(ret != NULL), in case future code changes cause the condition to becomes false. Include <assert.h>. --- src/xen/proxy_internal.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/xen/proxy_internal.c b/src/xen/proxy_internal.c index ec4522b..42b6e91 100644 --- a/src/xen/proxy_internal.c +++ b/src/xen/proxy_internal.c @@ -1,7 +1,7 @@ /* * proxy_client.c: client side of the communication with the libvirt proxy. * - * Copyright (C) 2006, 2008, 2009 Red Hat, Inc. + * Copyright (C) 2006, 2008, 2009, 2010 Red Hat, Inc. * * See COPYING.LIB for the License of this software * @@ -11,6 +11,7 @@ #include <config.h>
#include <stdio.h> +#include <assert.h> #include <stdlib.h> #include <unistd.h> #include <errno.h> @@ -444,7 +445,8 @@ retry: /* * do more checks on the incoming packet. */ - if ((res == NULL) || (res->version != PROXY_PROTO_VERSION) || + assert (res != NULL); + if ((res->version != PROXY_PROTO_VERSION) || (res->len < sizeof(virProxyPacket))) { virProxyError(conn, VIR_ERR_INTERNAL_ERROR, "%s", _("Communication error with proxy: malformed packet\n"));
I'm not too fond of adding an assert, res should not be null there or we should already have crashed. ACK to the change without the new assert line. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Daniel Veillard wrote:
On Wed, Jan 06, 2010 at 12:46:11PM +0100, Jim Meyering wrote:
As the log says, once we've dereferenced it, there's no point in comparing to NULL.
From 463eaf1027a154e71839a67eca85b3ada8b817ff Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Wed, 6 Jan 2010 12:45:07 +0100 Subject: [PATCH] don't test "res == NULL" after we've already dereferenced it
* src/xen/proxy_internal.c (xenProxyCommand): It is known to be non-NULL at that point, so remove the "ret == NULL" guard, and instead add an assert(ret != NULL), in case future code changes cause the condition to becomes false. Include <assert.h>. --- src/xen/proxy_internal.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/xen/proxy_internal.c b/src/xen/proxy_internal.c index ec4522b..42b6e91 100644 --- a/src/xen/proxy_internal.c +++ b/src/xen/proxy_internal.c @@ -1,7 +1,7 @@ /* * proxy_client.c: client side of the communication with the libvirt proxy. * - * Copyright (C) 2006, 2008, 2009 Red Hat, Inc. + * Copyright (C) 2006, 2008, 2009, 2010 Red Hat, Inc. * * See COPYING.LIB for the License of this software * @@ -11,6 +11,7 @@ #include <config.h>
#include <stdio.h> +#include <assert.h> #include <stdlib.h> #include <unistd.h> #include <errno.h> @@ -444,7 +445,8 @@ retry: /* * do more checks on the incoming packet. */ - if ((res == NULL) || (res->version != PROXY_PROTO_VERSION) || + assert (res != NULL); + if ((res->version != PROXY_PROTO_VERSION) || (res->len < sizeof(virProxyPacket))) { virProxyError(conn, VIR_ERR_INTERNAL_ERROR, "%s", _("Communication error with proxy: malformed packet\n"));
I'm not too fond of adding an assert, res should not be null there or we should already have crashed.
ACK to the change without the new assert line.
Considering that this is in the daemon and that "bad things" have been known to happen via NULL derefs, some would argue that an assertion failure is preferable. In addition, there's enough logic that it's not trivial to inspect the preceding code to be sure that "res" cannot be NULL -- hence it'd be easy for someone to change it without realizing they're causing trouble. Confirm that that's what you really want, and I'll remove the assert business and adjust the log message.
On Wed, Jan 06, 2010 at 09:55:23PM +0100, Jim Meyering wrote:
Daniel Veillard wrote:
On Wed, Jan 06, 2010 at 12:46:11PM +0100, Jim Meyering wrote:
As the log says, once we've dereferenced it, there's no point in comparing to NULL.
From 463eaf1027a154e71839a67eca85b3ada8b817ff Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Wed, 6 Jan 2010 12:45:07 +0100 Subject: [PATCH] don't test "res == NULL" after we've already dereferenced it
* src/xen/proxy_internal.c (xenProxyCommand): It is known to be non-NULL at that point, so remove the "ret == NULL" guard, and instead add an assert(ret != NULL), in case future code changes cause the condition to becomes false. Include <assert.h>. --- src/xen/proxy_internal.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/xen/proxy_internal.c b/src/xen/proxy_internal.c index ec4522b..42b6e91 100644 --- a/src/xen/proxy_internal.c +++ b/src/xen/proxy_internal.c @@ -1,7 +1,7 @@ /* * proxy_client.c: client side of the communication with the libvirt proxy. * - * Copyright (C) 2006, 2008, 2009 Red Hat, Inc. + * Copyright (C) 2006, 2008, 2009, 2010 Red Hat, Inc. * * See COPYING.LIB for the License of this software * @@ -11,6 +11,7 @@ #include <config.h>
#include <stdio.h> +#include <assert.h> #include <stdlib.h> #include <unistd.h> #include <errno.h> @@ -444,7 +445,8 @@ retry: /* * do more checks on the incoming packet. */ - if ((res == NULL) || (res->version != PROXY_PROTO_VERSION) || + assert (res != NULL); + if ((res->version != PROXY_PROTO_VERSION) || (res->len < sizeof(virProxyPacket))) { virProxyError(conn, VIR_ERR_INTERNAL_ERROR, "%s", _("Communication error with proxy: malformed packet\n"));
I'm not too fond of adding an assert, res should not be null there or we should already have crashed.
ACK to the change without the new assert line.
Considering that this is in the daemon and that "bad things" have been known to happen via NULL derefs, some would argue that an assertion failure is preferable.
No, this code is the client side of the Xen proxy implementation, that is never used within daemon context. 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 :|
Daniel P. Berrange wrote:
On Wed, Jan 06, 2010 at 09:55:23PM +0100, Jim Meyering wrote:
Daniel Veillard wrote:
On Wed, Jan 06, 2010 at 12:46:11PM +0100, Jim Meyering wrote:
As the log says, once we've dereferenced it, there's no point in comparing to NULL.
From 463eaf1027a154e71839a67eca85b3ada8b817ff Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Wed, 6 Jan 2010 12:45:07 +0100 Subject: [PATCH] don't test "res == NULL" after we've already dereferenced it
* src/xen/proxy_internal.c (xenProxyCommand): It is known to be non-NULL at that point, so remove the "ret == NULL" guard, and instead add an assert(ret != NULL), in case future code changes cause the condition to becomes false. Include <assert.h>. --- src/xen/proxy_internal.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/xen/proxy_internal.c b/src/xen/proxy_internal.c index ec4522b..42b6e91 100644 --- a/src/xen/proxy_internal.c +++ b/src/xen/proxy_internal.c @@ -1,7 +1,7 @@ /* * proxy_client.c: client side of the communication with the libvirt proxy. * - * Copyright (C) 2006, 2008, 2009 Red Hat, Inc. + * Copyright (C) 2006, 2008, 2009, 2010 Red Hat, Inc. * * See COPYING.LIB for the License of this software * @@ -11,6 +11,7 @@ #include <config.h>
#include <stdio.h> +#include <assert.h> #include <stdlib.h> #include <unistd.h> #include <errno.h> @@ -444,7 +445,8 @@ retry: /* * do more checks on the incoming packet. */ - if ((res == NULL) || (res->version != PROXY_PROTO_VERSION) || + assert (res != NULL); + if ((res->version != PROXY_PROTO_VERSION) || (res->len < sizeof(virProxyPacket))) { virProxyError(conn, VIR_ERR_INTERNAL_ERROR, "%s", _("Communication error with proxy: malformed packet\n"));
I'm not too fond of adding an assert, res should not be null there or we should already have crashed.
ACK to the change without the new assert line.
Considering that this is in the daemon and that "bad things" have been known to happen via NULL derefs, some would argue that an assertion failure is preferable.
No, this code is the client side of the Xen proxy implementation, that is never used within daemon context.
Oh, right. Thanks. However, the point is still valid, so I'll wait for confirmation. This is still about defensive coding, i.e., ensuring that maintenance doesn't violate invariants in harder-to-diagnose ways. If you get a bug report, which would you rather hear? "libvirt sometimes segfaults" or "I get an assertion failure on file.c:999"
On Thu, Jan 07, 2010 at 09:37:57AM +0100, Jim Meyering wrote:
Daniel P. Berrange wrote:
Considering that this is in the daemon and that "bad things" have been known to happen via NULL derefs, some would argue that an assertion failure is preferable.
No, this code is the client side of the Xen proxy implementation, that is never used within daemon context.
Oh, right. Thanks.
However, the point is still valid, so I'll wait for confirmation. This is still about defensive coding, i.e., ensuring that maintenance doesn't violate invariants in harder-to-diagnose ways. If you get a bug report, which would you rather hear? "libvirt sometimes segfaults" or "I get an assertion failure on file.c:999"
I guess it's mostly a matter of coding style, I'm not sure it makes such a difference in practice, libvirt output will likely be burried in a log file, unless virsh or similar CLI tool is used. We have only 4 asserts in the code currently, I think it shows that so far we are not relying on it. On one hand this mean calling exit() and I prefer a good old core dump for debugging than a one line message, on the other hand if you managed to catch that message, well this can help pinpoint if the person reporting has no debugging skills. I think there is pros and cons and that the current status quo is that we don't use asserts but more defensing coding by erroring out when this happen. The best way is not to assert() when we may dereference a NULL pointer but check for NULL at the point where we get that pointer, that's closer to the current code practice of libvirt (or well I expect so :-) and allow useful reporting (we failed to do a given operation) and a graceful error handling without exit'ing. So in general if we think we need an assert somewhere that mean that we failed to do the check at the right place, and I would rather not start to get into the practice of just asserting in a zillion place instead of checking at the correct place. So in my opinion, I'm still not fond of assert(), and I would prefer to catch up problem earlier, like parameter checking on function entry points or checking return value for functions producing pointers. In that case, res being NULL means that both answer and request parameters are null after the retry: label, since the two pointers are not modified in the function this should be tested when entering the function, if ((answer == NULL) && (request == NULL)) { virProxyError (NULL, VIR_ERR_INVALID_ARG, __FUNCTION__); return -1; } you get the error location, as in the assert but propagate the error back in the stack cleanly instead of exit'ing Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Thu, Jan 07, 2010 at 11:03:23AM +0100, Daniel Veillard wrote:
On Thu, Jan 07, 2010 at 09:37:57AM +0100, Jim Meyering wrote:
However, the point is still valid, so I'll wait for confirmation. This is still about defensive coding, i.e., ensuring that maintenance doesn't violate invariants in harder-to-diagnose ways. If you get a bug report, which would you rather hear? "libvirt sometimes segfaults" or "I get an assertion failure on file.c:999"
I guess it's mostly a matter of coding style, I'm not sure it makes such a difference in practice, libvirt output will likely be burried in a log file, unless virsh or similar CLI tool is used. We have only 4 asserts in the code currently, I think it shows that so far we are not relying on it. On one hand this mean calling exit() and I prefer a good old core dump for debugging than a one line message, on the other hand if you managed to catch that message, well this can help pinpoint if the person reporting has no debugging skills.
I think there is pros and cons and that the current status quo is that we don't use asserts but more defensing coding by erroring out when this happen. The best way is not to assert() when we may dereference a NULL pointer but check for NULL at the point where we get that pointer, that's closer to the current code practice of libvirt (or well I expect so :-) and allow useful reporting (we failed to do a given operation) and a graceful error handling without exit'ing. So in general if we think we need an assert somewhere that mean that we failed to do the check at the right place, and I would rather not start to get into the practice of just asserting in a zillion place instead of checking at the correct place.
So in my opinion, I'm still not fond of assert(), and I would prefer to catch up problem earlier, like parameter checking on function entry points or checking return value for functions producing pointers.
The other reason for adding assert(), is if the code leading upto a particular point is too complex to reliably understand whether a variable is NULL. I think that applies in this case. I don't think adding an assert() is the right way to deal with that complexity though. I think it is better to make the code clearer/easier to follow & understand 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 :|
Daniel P. Berrange wrote:
On Thu, Jan 07, 2010 at 11:03:23AM +0100, Daniel Veillard wrote:
On Thu, Jan 07, 2010 at 09:37:57AM +0100, Jim Meyering wrote:
However, the point is still valid, so I'll wait for confirmation. This is still about defensive coding, i.e., ensuring that maintenance doesn't violate invariants in harder-to-diagnose ways. If you get a bug report, which would you rather hear? "libvirt sometimes segfaults" or "I get an assertion failure on file.c:999"
I guess it's mostly a matter of coding style, I'm not sure it makes such a difference in practice, libvirt output will likely be burried in a log file, unless virsh or similar CLI tool is used. We have only 4 asserts in the code currently, I think it shows that so far we are not relying on it. On one hand this mean calling exit() and I prefer a good old core dump for debugging than a one line message, on the other hand if you managed to catch that message, well this can help pinpoint if the person reporting has no debugging skills.
I think there is pros and cons and that the current status quo is that we don't use asserts but more defensing coding by erroring out when this happen. The best way is not to assert() when we may dereference a NULL pointer but check for NULL at the point where we get that pointer, that's closer to the current code practice of libvirt (or well I expect so :-) and allow useful reporting (we failed to do a given operation) and a graceful error handling without exit'ing. So in general if we think we need an assert somewhere that mean that we failed to do the check at the right place, and I would rather not start to get into the practice of just asserting in a zillion place instead of checking at the correct place.
So in my opinion, I'm still not fond of assert(), and I would prefer to catch up problem earlier, like parameter checking on function entry points or checking return value for functions producing pointers.
The other reason for adding assert(), is if the code leading upto a particular point is too complex to reliably understand whether a variable is NULL. I think that applies in this case. I don't think adding an assert() is the right way to deal with that complexity though. I think it is better to make the code clearer/easier to follow & understand
I agree and have looked at the code in question (the somewhat duplicated if/else blocks in proxy_internal.c lines 385-443) http://libvirt.org/git/?p=libvirt.git;a=blob;f=src/xen/proxy_internal.c#l385 and don't see off hand why these if-blocks differ: res = request; if (res->len != sizeof(virProxyPacket)) { virProxyError(conn, VIR_ERR_INTERNAL_ERROR, _("Communication error with proxy: expected %d bytes got %d\n"), (int) sizeof(virProxyPacket), res->len); goto error; } res = (virProxyPacketPtr) answer; if ((res->len < sizeof(virProxyPacket)) || (res->len > sizeof(virProxyFullPacket))) { virProxyError(conn, VIR_ERR_INTERNAL_ERROR, _("Communication error with proxy: got %d bytes packet\n"), res->len); goto error; }
Daniel Veillard wrote: ...
However, the point is still valid, so I'll wait for confirmation. This is still about defensive coding, i.e., ensuring that maintenance doesn't violate invariants in harder-to-diagnose ways. If you get a bug report, which would you rather hear? "libvirt sometimes segfaults" or "I get an assertion failure on file.c:999"
I guess it's mostly a matter of coding style, I'm not sure it makes such a difference in practice, libvirt output will likely be burried in a log file, unless virsh or similar CLI tool is used. We have only 4 asserts in the code currently, I think it shows that so far we are not relying on it. On one hand this mean calling exit()
Is "don't use assert" libvirt policy? I hope not.
and I prefer a good old core dump for debugging than a one line message,
Same here, but there are many reasons for which a reporter will be unwilling or unable to provide a core dump. No one hesitates to include the output of a failed assertion in a bug report.
on the other hand if you managed to catch that message, well this can help pinpoint if the person reporting has no debugging skills.
I think there is pros and cons and that the current status quo is that we don't use asserts but more defensing coding by erroring out when this happen. The best way is not to assert() when we may dereference a NULL pointer but check for NULL at the point where we get that pointer, that's closer to the current code practice of libvirt (or well I expect so :-) and allow useful reporting (we failed to do a given operation) and a graceful error handling without exit'ing. So in general if we think we need an assert somewhere that mean that we failed to do the check at the right place, and I would
No. That is not how one should use assert, and certainly not how I proposed to use it. This is not about testing for a user- or system-provokable condition, but rather about testing code invariants. See below.
rather not start to get into the practice of just asserting in a zillion place instead of checking at the correct place.
So in my opinion, I'm still not fond of assert(), and I would prefer to catch up problem earlier, like parameter checking on function entry points or checking return value for functions producing pointers.
In that case, res being NULL means that both answer and request parameters are null after the retry: label, since the two pointers are not modified in the function this should be tested when entering the function,
if ((answer == NULL) && (request == NULL)) { virProxyError (NULL, VIR_ERR_INVALID_ARG, __FUNCTION__); return -1; }
you get the error location, as in the assert but propagate the error back in the stack cleanly instead of exit'ing
It is important to distinguish two types of errors: - conditions that may arise due to user input or environment (misbehaving client or server, malformed packet, I/O error, etc.) - internal API abuse, violation of an internal assumption or invariant Using "assert" is appropriate only in the latter case, for detecting conditions that typically are provoked only by developer-introduced errors. Adding error-handling code like the above is appropriate only in the first case. Somewhat along these lines, we are starting to remove certain types of tests, (e.g., conn == NULL), when "conn" is always non-NULL. In the case of this patch, "request" is always non-null, and should probably have the "nonnull" attribute. New patch appended below. One advantage of using assert is that it usually _reduces_ the maintenance burden (i.e., when skimming the code, you may ignore the assert statement). However, if you add expressions like the above to "ensure" a condition that will always be true (i.e., that should not be possible to trigger via user input), you *increase* the maintenance burden by adding a test of an always-false condition that is handled as if it *can* sometimes be true. That would increase the WTF/m rate for certain readers. (http://www.osnews.com/story/19266/WTFs_m) Such a test might even trigger new warnings from tools like clang and coverity. Whether we use an assertion in this particular case is not important, but I hope that libvirt adopts a judicious policy on the use of "assert". Here's the revised patch, followed by the tiny nonnull-one. (also fixes typos s/ret/res/ in log message)
From 2a7fed9238667ff75a6ecd662b663549bc8fb7b5 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Wed, 6 Jan 2010 12:45:07 +0100 Subject: [PATCH] don't test "res == NULL" after we've already dereferenced "res"
* src/xen/proxy_internal.c (xenProxyCommand): "res" is known to be non-NULL at that point, so remove the "res == NULL" guard. --- src/xen/proxy_internal.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/xen/proxy_internal.c b/src/xen/proxy_internal.c index ec4522b..ee4678a 100644 --- a/src/xen/proxy_internal.c +++ b/src/xen/proxy_internal.c @@ -1,7 +1,7 @@ /* * proxy_client.c: client side of the communication with the libvirt proxy. * - * Copyright (C) 2006, 2008, 2009 Red Hat, Inc. + * Copyright (C) 2006, 2008, 2009, 2010 Red Hat, Inc. * * See COPYING.LIB for the License of this software * @@ -444,7 +444,7 @@ retry: /* * do more checks on the incoming packet. */ - if ((res == NULL) || (res->version != PROXY_PROTO_VERSION) || + if ((res->version != PROXY_PROTO_VERSION) || (res->len < sizeof(virProxyPacket))) { virProxyError(conn, VIR_ERR_INTERNAL_ERROR, "%s", _("Communication error with proxy: malformed packet\n")); -- 1.6.6.425.g4dc2d
From eb6f7f0478ce510de8dc5fc9add4eaaca1c2bfc1 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 7 Jan 2010 19:48:05 +0100 Subject: [PATCH] proxy_internal.c: mark "request" parameter as nonnull
* src/xen/proxy_internal.c (xenProxyCommand): Mark "request" as an always-non-NULL parameter. --- src/xen/proxy_internal.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/xen/proxy_internal.c b/src/xen/proxy_internal.c index ee4678a..73a0469 100644 --- a/src/xen/proxy_internal.c +++ b/src/xen/proxy_internal.c @@ -340,7 +340,7 @@ xenProxyClose(virConnectPtr conn) return 0; } -static int +static int ATTRIBUTE_NONNULL(2) xenProxyCommand(virConnectPtr conn, virProxyPacketPtr request, virProxyFullPacketPtr answer, int quiet) { static int serial = 0; -- 1.6.6.425.g4dc2d
On Fri, Jan 08, 2010 at 12:13:24PM +0100, Jim Meyering wrote:
Daniel Veillard wrote: ...
However, the point is still valid, so I'll wait for confirmation. This is still about defensive coding, i.e., ensuring that maintenance doesn't violate invariants in harder-to-diagnose ways. If you get a bug report, which would you rather hear? "libvirt sometimes segfaults" or "I get an assertion failure on file.c:999"
I guess it's mostly a matter of coding style, I'm not sure it makes such a difference in practice, libvirt output will likely be burried in a log file, unless virsh or similar CLI tool is used. We have only 4 asserts in the code currently, I think it shows that so far we are not relying on it. On one hand this mean calling exit()
Is "don't use assert" libvirt policy? I hope not.
The policy is "use defensive programming rather than exit or crash" this means that assert has his place only in very untractable cases.
It is important to distinguish two types of errors:
- conditions that may arise due to user input or environment (misbehaving client or server, malformed packet, I/O error, etc.)
- internal API abuse, violation of an internal assumption or invariant
Using "assert" is appropriate only in the latter case, for detecting conditions that typically are provoked only by developer-introduced errors.
Hum, that's where we disagree. If libvirtd goes down because the configuration for one VM used a little used construct which happened to trigger a bug, exiting gives troubles to all the running domains. We must be more permissive to developer-introduced errors than for a single user program instance. The key point is that the error is being reported while avoiding crashing.
Here's the revised patch, followed by the tiny nonnull-one. (also fixes typos s/ret/res/ in log message)
From 2a7fed9238667ff75a6ecd662b663549bc8fb7b5 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Wed, 6 Jan 2010 12:45:07 +0100 Subject: [PATCH] don't test "res == NULL" after we've already dereferenced "res"
* src/xen/proxy_internal.c (xenProxyCommand): "res" is known to be non-NULL at that point, so remove the "res == NULL" guard. --- src/xen/proxy_internal.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/xen/proxy_internal.c b/src/xen/proxy_internal.c index ec4522b..ee4678a 100644 --- a/src/xen/proxy_internal.c +++ b/src/xen/proxy_internal.c @@ -1,7 +1,7 @@ /* * proxy_client.c: client side of the communication with the libvirt proxy. * - * Copyright (C) 2006, 2008, 2009 Red Hat, Inc. + * Copyright (C) 2006, 2008, 2009, 2010 Red Hat, Inc. * * See COPYING.LIB for the License of this software * @@ -444,7 +444,7 @@ retry: /* * do more checks on the incoming packet. */ - if ((res == NULL) || (res->version != PROXY_PROTO_VERSION) || + if ((res->version != PROXY_PROTO_VERSION) || (res->len < sizeof(virProxyPacket))) { virProxyError(conn, VIR_ERR_INTERNAL_ERROR, "%s", _("Communication error with proxy: malformed packet\n")); -- 1.6.6.425.g4dc2d
From eb6f7f0478ce510de8dc5fc9add4eaaca1c2bfc1 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 7 Jan 2010 19:48:05 +0100 Subject: [PATCH] proxy_internal.c: mark "request" parameter as nonnull
* src/xen/proxy_internal.c (xenProxyCommand): Mark "request" as an always-non-NULL parameter. --- src/xen/proxy_internal.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/xen/proxy_internal.c b/src/xen/proxy_internal.c index ee4678a..73a0469 100644 --- a/src/xen/proxy_internal.c +++ b/src/xen/proxy_internal.c @@ -340,7 +340,7 @@ xenProxyClose(virConnectPtr conn) return 0; }
-static int +static int ATTRIBUTE_NONNULL(2) xenProxyCommand(virConnectPtr conn, virProxyPacketPtr request, virProxyFullPacketPtr answer, int quiet) { static int serial = 0; -- 1.6.6.425.g4dc2d
ACK, fine by me, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Jim Meyering