Hi Sarah,
See comments in line
Joseph J. VLcek wrote:
> 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?
I think what Joe is specifically referring to is the "i"th instance,
i.e. when dp->controllers[i]->name matches cp->name.
In this case, we overwrite dp->controllers[i] with i+1 (referred to as
"j" with "j+1"). At this point any reference to matched instance from
dp->controllers[i] is lost.
So the assertion made is that it points cp, which is freed below.
Is that correct ?
-Sanjay
>>>>>
>>>> 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
>>>
>>>
>>>
>
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
