sanjay nadkarni (Laptop) wrote:
> 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
Yes that was my point... but my understanding from what Sarah has
described is that the memory is properly cleaned up.
Joe
>
>
>>>>>>
>>>>> 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
>
