Thank you Sarah!
Sarah Jelinek wrote:
>
>
> 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
>>
>>
>>
