[libvirt] [PATCH] don't test "res == NULL" after we've already dereferenced it

[Daniel Veillard]

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 at 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 at 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 at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/