[libvirt] [PATCH] xenDaemonDomainSetAutostart: avoid appearance of impropriety

[Daniel Veillard]

On Thu, Feb 18, 2010 at 07:20:17AM +0100, Jim Meyering wrote:
> Coverity noticed that of the 13 uses of sexpr_lookup,
> only this one was unchecked, and here, the result is dereferenced.
> It happens to be a false positive, due to the preceding sexpr_node
> check, but worth fixing: sexpr_node is just a very thin wrapper
> around sexpr_lookup so there's little justification for looking
> up the same string twice.
> 
> >From a9ab34214cf9d247d39731563dcc70b8f1dc73b5 Mon Sep 17 00:00:00 2001
> From: Jim Meyering <meyering at redhat.com>
> Date: Wed, 17 Feb 2010 22:14:25 +0100
> Subject: [PATCH] xenDaemonDomainSetAutostart: avoid appearance of impropriety
> 
> * src/xen/xend_internal.c (xenDaemonDomainSetAutostart): Rewrite to
> avoid dereferencing the result of sexpr_lookup.  While in this
> particular case, it was guaranteed never to be NULL, due to the
> preceding "if sexpr_node(...)" guard, it's cleaner to skip the
> sexpr_node call altogether, and also saves a lookup.
> ---
>  src/xen/xend_internal.c |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c
> index 88923c8..1f8b106 100644
> --- a/src/xen/xend_internal.c
> +++ b/src/xen/xend_internal.c
> @@ -4383,7 +4383,6 @@ xenDaemonDomainSetAutostart(virDomainPtr domain,
>                              int autostart)
>  {
>      struct sexpr *root, *autonode;
> -    const char *autostr;
>      char buf[4096];
>      int ret = -1;
>      xenUnifiedPrivatePtr priv;
> @@ -4408,16 +4407,17 @@ xenDaemonDomainSetAutostart(virDomainPtr domain,
>          return (-1);
>      }
> 
> -    autostr = sexpr_node(root, "domain/on_xend_start");
> -    if (autostr) {
> -        if (!STREQ(autostr, "ignore") && !STREQ(autostr, "start")) {
> +    autonode = sexpr_lookup(root, "domain/on_xend_start");
> +    if (autonode) {
> +        const char *val = (autonode->u.s.car->kind == SEXPR_VALUE
> +                           ? autonode->u.s.car->u.value : NULL);
> +        if (!STREQ(val, "ignore") && !STREQ(val, "start")) {
>              virXendError(domain->conn, VIR_ERR_INTERNAL_ERROR,
>                           "%s", _("unexpected value from on_xend_start"));
>              goto error;
>          }
> 
>          // Change the autostart value in place, then define the new sexpr
> -        autonode = sexpr_lookup(root, "domain/on_xend_start");
>          VIR_FREE(autonode->u.s.car->u.value);
>          autonode->u.s.car->u.value = (autostart ? strdup("start")
>                                                  : strdup("ignore"));

  ACK,

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/