5:46 a.m.
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(a)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
11:24 a.m.
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(a)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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
2:55 p.m.
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(a)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.
2:28 a.m.
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(a)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 :|
2:37 a.m.
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(a)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"
4:03 a.m.
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
4:27 a.m.
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 :|
5:59 a.m.
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;
}
5:13 a.m.
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(a)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(a)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
7:30 a.m.
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(a)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(a)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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
5408
days inactive
5410
days old
9 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Jim Meyering