Re: [libvirt] [PATCH] fix xenDaemonListDefinedDomains
Matthias Bolte wrote:
2009/11/25 Jim Fehlig <jfehlig(a)novell.com>:
> Commit 790f0b3057787bb64da8c46c111ff8d3eff7b2af causes contents of names
> array to be freed even on success, resulting in no listing of defined
> but inactive Xen domains. Patch below fixes it.
>
> Regards,
> Jim
>
>
Good catch, I just reviewed this commit to see if I've caused similar
bugs elsewhere, but this seems to be the only one.
I had looked elsewhere in that commit as well but didn't see any other
problems.
> Index: libvirt-0.7.4/src/xen/xend_internal.c
> ===================================================================
> --- libvirt-0.7.4.orig/src/xen/xend_internal.c
> +++ libvirt-0.7.4/src/xen/xend_internal.c
> @@ -4693,13 +4693,14 @@ xenDaemonListDefinedDomains(virConnectPt
> }
>
> if (ret >= maxnames)
> - break;
> + goto out;
> }
>
> error:
> for (i = 0; i < ret; ++i)
> VIR_FREE(names[i]);
>
> +out:
> sexpr_free(root);
> return(ret);
> }
>
Your patch doesn't fix the problem in all situations. If maxnames is
larger than the actual number of domains then goto out is never
executed.
Ah, yes - good catch.
I also forgot to set ret to -1 after freeing the names, this needs
to
be fixed too.
I propose the attached patch to solve this issues.
Matthias
diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c
index d61e9e6..a0c7d77 100644
--- a/src/xen/xend_internal.c
+++ b/src/xen/xend_internal.c
@@ -4696,12 +4696,17 @@ xenDaemonListDefinedDomains(virConnectPtr conn, char **const
names, int maxnames
break;
}
+cleanup:
+ sexpr_free(root);
+ return(ret);
+
error:
for (i = 0; i < ret; ++i)
VIR_FREE(names[i]);
- sexpr_free(root);
- return(ret);
+ ret = -1;
+
+ goto cleanup;
}
/**
ACK. Thanks again!
Jim