Matthias Bolte wrote:
2009/11/25 Jim Fehlig <jfehlig@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