[reordering - top-posting is hard to follow]
On 01/30/2012 08:11 AM, Alex Jia wrote:
>> @@ -530,10 +532,6 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
>>
>> for (i = 0; i < dns->nsrvrecords; i++) {
>> char *record = NULL;
>> - char *recordPort = NULL;
>> - char *recordPriority = NULL;
>> - char *recordWeight = NULL;
>> -
>> if (dns->srvrecords[i].service && dns->srvrecords[i].protocol) {
>> if (dns->srvrecords[i].port) {
>> if (virAsprintf(&recordPort, "%d", dns->srvrecords[i].port) < 0) {
>> @@ -671,6 +669,11 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
>
> I think we need v4; You want to free memory in the very same loop where
> it is allocated. Otherwise we still leak memory from previous loop
> iterations. More detailed, virAsprintf() takes given pointer and
> overwrites it. So if pointer was referring to an allocated memory, we
> lost this single reference and thus leak.
>
> Michal
> Hi Michal,
> Thanks for your comment, the following v1 patch should be a right way?
> https://www.redhat.com/archives/libvir-list/2012-January/msg00209.html
v1 was incomplete, but was at least attempting to free things in the for
loop where they were allocated.
What I'd suggest is:
declare variables outside the loop,
start the for loop by freeing any state from previous iterations,
and also free all state in the cleanup label
at which point, you _don't_ have to follow the v1 approach of trying to
free variables before every goto or break (and missing some). Something
like:
char *record = NULL;
for (i = 0; i < dns->nsrrvrecords; i++) {
/* free previous iteration */
VIR_FREE(record);
...
record = ...
if (error)
goto cleanup;
}
cleanup:
VIR_FREE(record);
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list