[Ns-developers] Review application

[Ismail Amine]

Faker,

A regression was introduced in the code, I'm no longer able to execute 
the wimax-ipv4 example,,,it exits with a core dump!!
the same thing happens also for wimax-multicast.
We sould execute at least the 2 tests I described in a previous mail and 
check that they output  OK before uploading  a  new version in the 
mercurial  repository.

Thank you

Best regards
Amine

Faker Moatamri wrote:
> Ismail Amine wrote:
>> Faker Moatamri wrote:
>>> Mathieu Lacage wrote:
>>>> [trimming CC list]
>>>>
>>>> On Tue, 2009-08-25 at 16:57 +0200, Ismail Amine wrote:
>>>>
>>>>  
>>>>>> I have also some questions regarding the code:
>>>>>>
>>>>>> -file bandwidth manager.cc line 238, the variables 
>>>>>> rtpsAllocationPerFrame = 0, nrtpsAllocationPerFrame = 0, 
>>>>>> beAllocationPerFrame = 0; are set to 0 and never updated, so 
>>>>>> basically they don't add anything to the return value:
>>>>>> uint32_t allocatedSymbols = ugsAllocationPerFrame + 
>>>>>> rtpsAllocationPerFrame
>>>>>>      + nrtpsAllocationPerFrame + beAllocationPerFrame;
>>>>>> return allocatedSymbols;
>>>>>> Only the value of ugsAllocationPerFrame is updated, the other 
>>>>>> values remain 0, can I remove those useless variables?
>>>>>>       
>>>>> Faker, these files have been written by Tippu. I didn't go into 
>>>>> implementation details but I think it's wiser not to change any 
>>>>> thing we don't know how to test...unless you're sure
>>>>>     
>>>>
>>>> If we have code without any tests, no maintainer, and it looks 
>>>> buggy, it
>>>> should be killed (err, not merged so that we don't have to kill it 
>>>> Faker
>>>> later :). But maybe someone could try to track down tippu and ask him
>>>> the question ?
>>>>
>>>>   
>>> Actually this is not a bug but rather an non useful variables that 
>>> have significant names for wimax people I guess, their only matter 
>>> is that they are not updated, having an answer or a document about 
>>> it would be cool. Does rtpsAllocationPerFrame, 
>>> nrtpsAllocationPerFrame, beAllocationPerFrame mean something for 
>>> Wimax experts? If they mean nothing then we will remove them but I 
>>> think they should mean something.
>>>
>>
>> I had a look on the function 
>> BandwidthManager::GetSymbolsPerFrameAllocated (void), and I confirm 
>> that the variables rtpsAllocationPerFrame, nrtpsAllocationPerFrame, 
>> beAllocationPerFrame should be removed and the variable 
>> ugsAllocationPerFrame should be renamed allocationPerFrame.  I will 
>> modify and include it in my next patch!
> Done
>>> -In wimax-ss-net-device.cc there is a lot of new statements, are 
>>> they deleted somewhere?
>>>>>>       
>>>>> Can you be more precise?
>>>>>     
>>>>
>>>> Where are the delete statements which correspond to the new statements
>>>> found in that function ? Are we leaking objects ?
>>>>
>>>>   
>>> in the constructor:
>>> WimaxSubscriberStationNetDevice::WimaxSubscriberStationNetDevice 
>>> (void) :
>>>      ......., m_dlBurstProfile (
>>>          new OfdmDlBurstProfile ()), m_ulBurstProfile (
>>>          new OfdmUlBurstProfile ()), .....
>>>
>>> {
>>>  .....
>>>  m_classifier = new IpcsSSPacketclassifier (m_serviceFlowManager);
>>>  m_linkManager = new SSLinkManager (this);
>>>  m_scheduler = new SSScheduler (this);
>>> }
>>> The destructor is empty:
>>> WimaxSubscriberStationNetDevice::~WimaxSubscriberStationNetDevice 
>>> (void)
>>> {
>>> }
>>> and we have the DoDispose function:
>>> void
>>> WimaxSubscriberStationNetDevice::DoDispose (void)
>>> {
>>> ...
>>>  delete m_dlBurstProfile;
>>>  delete m_ulBurstProfile;
>>>  delete m_scheduler;
>>>  delete m_linkManager;
>>>  m_dlBurstProfile = 0;
>>>  m_ulBurstProfile = 0;
>>>  m_scheduler = 0;
>>>  m_linkManager = 0;
>>>
>>>  WimaxNetDevice::DoDispose ();
>>> }
>>> From what I see here we have the /m_classifier/ object that is never 
>>> emptied unless it is deleted somewhere else. I know it's easy to fix 
>>> if it is a bug but please verify that all the new statements are 
>>> followed somewhere by a delete statement and that the objects 
>>> created using <CreateObject> is unreferenced somewhere after.
>>
>> I confirm that it is a bug, I will fix it for the next patch
> done
>