[libvirt] [PATCH 0/3] Adjust some formatting for numa distance code and resolve a memory leak
While reviewing the recent distance adjustments for Michal, I noted a couple of minor formatting things as well as a memory leak when removing the distances. John Ferlan (3): conf: Clean up virDomainNumaDefNodeDistanceParseXML conf: Clean up virDomainNumaDefCPUFormatXML conf: Fix memory leak for distances in virDomainNumaFree src/conf/numa_conf.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) -- 2.13.6
Clean up the style a bit w/r/t to not using a unary operator on an integer value that could be zero - compare vs. zero instead. Set the def->mem_nodes[*].distances to rdist or ldist inside the if condition - no need to set outside since the value being set to is what was fetched. During cleanup, be sure to initialize the ndistances on error and use the < 0 comparison not the unary one. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/numa_conf.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index 8fc3b0a196..3aae705a5d 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -714,7 +714,7 @@ virDomainNumaDefNodeDistanceParseXML(virDomainNumaPtr def, xmlNodePtr *nodes = NULL; size_t i, ndistances = def->nmem_nodes; - if (!ndistances) + if (ndistances == 0) return 0; /* check if NUMA distances definition is present */ @@ -805,11 +805,11 @@ virDomainNumaDefNodeDistanceParseXML(virDomainNumaPtr def, ldist[cur_cell].value = LOCAL_DISTANCE; ldist[cur_cell].cellid = cur_cell; def->mem_nodes[cur_cell].ndistances = ndistances; + def->mem_nodes[cur_cell].distances = ldist; } ldist[sibling_id].cellid = sibling_id; ldist[sibling_id].value = sibling_value; - def->mem_nodes[cur_cell].distances = ldist; /* Apply symmetry if none given */ rdist = def->mem_nodes[sibling_id].distances; @@ -820,20 +820,21 @@ virDomainNumaDefNodeDistanceParseXML(virDomainNumaPtr def, rdist[sibling_id].value = LOCAL_DISTANCE; rdist[sibling_id].cellid = sibling_id; def->mem_nodes[sibling_id].ndistances = ndistances; + def->mem_nodes[sibling_id].distances = rdist; } rdist[cur_cell].cellid = cur_cell; if (!rdist[cur_cell].value) rdist[cur_cell].value = sibling_value; - def->mem_nodes[sibling_id].distances = rdist; } ret = 0; cleanup: - if (ret) { + if (ret < 0) { for (i = 0; i < ndistances; i++) VIR_FREE(def->mem_nodes[i].distances); + def->mem_nodes[i].ndistances = 0; } VIR_FREE(nodes); VIR_FREE(tmp); -- 2.13.6
Don't use a unary comparison for an int value - compare against zero directly instead. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/numa_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index 3aae705a5d..eadf8f2282 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -992,7 +992,7 @@ virDomainNumaDefCPUFormatXML(virBufferPtr buf, virDomainMemoryAccessTypeToString(memAccess)); ndistances = def->mem_nodes[i].ndistances; - if (!ndistances) { + if (ndistances == 0) { virBufferAddLit(buf, "/>\n"); } else { size_t j; -- 2.13.6
On 11/24/2017 06:18 PM, John Ferlan wrote:
Don't use a unary comparison for an int value - compare against zero directly instead.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/numa_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index 3aae705a5d..eadf8f2282 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -992,7 +992,7 @@ virDomainNumaDefCPUFormatXML(virBufferPtr buf, virDomainMemoryAccessTypeToString(memAccess));
ndistances = def->mem_nodes[i].ndistances; - if (!ndistances) { + if (ndistances == 0) { virBufferAddLit(buf, "/>\n"); } else { size_t j;
You know that I don't care that much :-) But at the same time, if !int and int == 0 are equal to me, but you prefer one, we can use the one you prefer (since those two options are equal to me). Michal
Commit id '74119a03f' neglected to clean up @distances when the numa definition is cleaned up. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/numa_conf.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index eadf8f2282..c906a53de0 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -362,6 +362,9 @@ virDomainNumaFree(virDomainNumaPtr numa) for (i = 0; i < numa->nmem_nodes; i++) { virBitmapFree(numa->mem_nodes[i].cpumask); virBitmapFree(numa->mem_nodes[i].nodeset); + + if (numa->mem_nodes[i].ndistances > 0) + VIR_FREE(numa->mem_nodes[i].distances); } VIR_FREE(numa->mem_nodes); -- 2.13.6
ping? Patch 3 is a memory leak bugfix for code added in this release... Tks, John On 11/24/2017 12:18 PM, John Ferlan wrote:
While reviewing the recent distance adjustments for Michal, I noted a couple of minor formatting things as well as a memory leak when removing the distances.
John Ferlan (3): conf: Clean up virDomainNumaDefNodeDistanceParseXML conf: Clean up virDomainNumaDefCPUFormatXML conf: Fix memory leak for distances in virDomainNumaFree
src/conf/numa_conf.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
On 11/24/2017 06:18 PM, John Ferlan wrote:
While reviewing the recent distance adjustments for Michal, I noted a couple of minor formatting things as well as a memory leak when removing the distances.
John Ferlan (3): conf: Clean up virDomainNumaDefNodeDistanceParseXML conf: Clean up virDomainNumaDefCPUFormatXML conf: Fix memory leak for distances in virDomainNumaFree
src/conf/numa_conf.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
ACK series and safe for freeze. BTW: while reviewing this I've found couple of other memleaks. Will post patches shortly. Michal
participants (2)
-
John Ferlan -
Michal Privoznik