Joseph J. VLcek wrote:
> Sarah Jelinek wrote:
>>
>>
>> Joseph J. VLcek wrote:
>>> Sarah Jelinek wrote:
>>>> Hi Jan and Ginnie,
>>>>
>>>> Could you do a code review for:
>>>>
>>>> 6795293 libdiskmgt coredump on x86 with devices under mpxio
>>>> control/SUN-Universal Xport inbound mangement
>>>>
>>>> Webrev is here:
>>>>
>>>> http://cr.opensolaris.org/~sjelinek/6795293/
>>>>
>>>> ****Building and Testing done:
>>>> -Both sparc and x86 built clean
>>>> -Tested on both x86 and sparc:
>>>> x86 testing reproduced bug and also shows bug fix
>>>> sparc testing for regression testing. Bug does not present
>>>> itself on sparc.
>>>>
>>>> -Reproduced bug and tested fix on oaf627.ireland. This is an x86
>>>> box with Qlogic FC controllers.
>>>> -Tested both mpxio and non-mpxio
>>>> -ran ::findleaks. No leaks detected.
>>>>
>>>> Test output is located here:
>>>>
>>>> /net/irperf.ireland/export/work/bfu/sjelinek
>>>>
>>>> The files in this directory are specifically:
>>>> ***Bad run(bug is present):
>>>> -core.test_td.2664.1263495933.gz-core showing original bug reproduced
>>>> -libdiskmgt_bad.txt
>>>> -test_td_bad.txt
>>>>
>>>> ***Good run(bug fixed):
>>>> -libdiskmgt_good.txt
>>>> -test_td_good.txt
>>>> -format.txt - shows output of format to show match with
>>>> test_td_good.txt data.
>>>>
>>>> ***Good run, mpxio disabled(bug fixed):
>>>> -libdiskmgt_nonmpxio.txt
>>>> -test_td_nonmpxio.txt
>>>> -format_nonmpxio.txt - shows output of format to show match with
>>>> -test_td_nonmpxio.txt data.
>>>>
>>>>
>>>> thanks,
>>>> sarah
>>>>
>>>> _______________________________________________
>>>> caiman-discuss mailing list
>>>> caiman-discuss at opensolaris.org
>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>>> Sarah,
>>>
>>> This looks good to me.
>>>
>>> I have one questions.
>>>
>>> 869-884
>>> Is moving items up in the lists creating a memory leak?
>>>
>> Hi Joe,
>>
>> Thank you for the review.
>>
>> No, because I free that memory, with cache_free_controller() at line 897:
>>> 897 cache_free_controller(cp);
>>
>> All I am doing at lines 869-884 is moving the pointers in the Disk N
>> datastructure that point to the controllers associated with Disk N. I
>> do this first, then free the memory that is actually allocated to the
>> controller that we have just disassociated with Disk N.
> Thanks for the reply Sarah.
>
> Where is the memory freed? By the call to:
>
> 900 cache_free_controller(cp);
Yes, in the updated webrev, which has changed based on Jan's comments,
this is where the memory is freed.
This code does:
> void
> cache_free_controller(controller_t *cp)
> {
> free(cp->name);
> free(cp->kstat_name);
> free(cp->disks);
> if (cp->paths != NULL) {
> int i;
>
> for (i = 0; cp->paths[i]; i++) {
> /* free the path since it can't exist w/o the ctrlr */
> cache_free_path(cp->paths[i]);
> }
> free(cp->paths);
> }
>
> free(cp);
> }
It's in cache.c
thanks,
sarah
*****
>
> ?
>
> Joe
>
>
>
