Bugzilla – Bug 2442
TrafficControl makes ConfigStore crash
Last modified: 2016-06-29 12:32:13 UTC
The error has been reported here: https://groups.google.com/forum/#!topic/ns-3-users/TR9LT4bgeQg It is not limited to GtkConfigStore. Also "simply" saving the default object configs will make a program crash. As an example open any script and add the following: Config::SetDefault ("ns3::ConfigStore::Filename", StringValue ("output-attributes.txt")); Config::SetDefault ("ns3::ConfigStore::FileFormat", StringValue ("RawText")); Config::SetDefault ("ns3::ConfigStore::Mode", StringValue ("Save")); ConfigStore config;
I cannot reproduce the crashes with the current ns-3-dev (neither with ns-3.25). I added the lines you mention to a couple of examples (traffic-control and mixed-wireless) and they both run fine. I ran the lena-simple-epc example shipped with ns-3 (i.e., without GtkConfigStore enabled) and worked fine. The example attached by the user, which differs from the one shipped by ns-3 in that GtkConfigStore is enabled, crashes instead. Thus, it seems that GtkConfigStore actually does the difference. Any clue about possible reasons?
Created attachment 2481 [details] crash example This is an example of the crash with the plain ConfigStore
This attribute seems to be wrong (using wrong accessor methods): .AddAttribute ("RootQueueDiscList", "The list of root queue discs associated to this Traffic Control layer.", ObjectMapValue (), MakeObjectMapAccessor (&TrafficControlLayer::GetNDevices, &TrafficControlLayer::GetRootQueueDiscOnDeviceByIndex), MakeObjectMapChecker<QueueDisc> ()) The underlying type that this ObjectMap handles is a std::map <T, U > where U is a Ptr to an ns3::Object, but these methods do not handle that type. Here is how it is being used (note, I don't know whether this code is even working properly): Config::ConnectWithoutContext ("/NodeList/2/$ns3::TrafficControlLayer/RootQueueDiscList/0/$ns3::CoDelQueueDisc/DropState", MakeBoundCallback (&DroppingStateTracer, stream)); I think what is needed here is ObjectVectorValue such as how NodeList is done. It seems to me that the vector that you want here is that RootQueueDiscList is a std::vector<Ptr<QueueDisc> > indexed by NetDevice index, so you probably need to create and maintain that member variable and then clone the approach done for NodeList.
Created attachment 2484 [details] Fix for AttributeIterator There are actually two bugs in my opinion. One is in AttributeIterator::DoIterate when handling an attribute that is an object container: NS_LOG_DEBUG ("ObjectPtrContainer attribute " << info.name); ObjectPtrContainerValue vector; object->GetAttribute (info.name, vector); StartVisitArrayAttribute (object, info.name, vector); ObjectPtrContainerValue::Iterator it; for (it = vector.Begin (); it != vector.End (); ++it) { uint32_t j = (*it).first; NS_LOG_DEBUG ("ObjectPtrContainer attribute item " << j); Ptr<Object> tmp = (*it).second; --> StartVisitArrayItem (vector, j, tmp); m_examined.push_back (object); DoIterate (tmp); m_examined.pop_back (); EndVisitArrayItem (); } EndVisitArrayAttribute (); continue; If an attribute is a container of Ptr<Object>, I think it is perfectly fine that one item may be a null pointer. In such a case, tmp is null and StartVisitArrayItem segfaults when doing item->GetInstanceTypeId (). This case is triggered by RootQueueDiscList when no queue disc is installed on a netdevice (see next comment). I attach a simple fix, which I verified avoids that the script attached by Tommaso crashes.
Created attachment 2485 [details] Fix for traffic control The second bug is in the traffic control. I believe the definition of the accessor functions is correct: template <typename T, typename U, typename INDEX> Ptr<const AttributeAccessor> MakeObjectMapAccessor (Ptr<U> (T::*get)(INDEX) const, INDEX (T::*getN)(void) const); In TrafficControlLayer, we have: Ptr<QueueDisc> GetRootQueueDiscOnDeviceByIndex (uint32_t index) const; uint32_t GetNDevices (void) const; thus: T = TrafficControlLayer U = QueueDisc INDEX = uint32_t My idea was to have a map<Ptr<NetDevice>, NetDeviceInfo> (which is TrafficControlLayer::m_netDevices) so that TrafficControlLayer::Send (Ptr<NetDevice> device, Ptr<QueueDiscItem> item) can directly use device to access the map and find the queue disc where the item must be enqueued. On the other hand, I wanted to allow users to access the traced attributes of queue discs by using the config system: Config::ConnectWithoutContext ("/NodeList/1/$ns3::TrafficControlLayer/RootQueueDiscList/0/PacketsInQueue", MakeCallback (&TcPacketsInQueueTrace)); Here, the user provides the id of a netdevice (its id in NodeList) and gets the root queue disc installed on that device. This is done by means of the RootQueueDiscList attribute, which therefore must be an object container storing m_node->GetNDevices () items. The i-th item stores the root queue disc installed on the i-th (as in NodeList) device. This is correctly done: Ptr<QueueDisc> TrafficControlLayer::GetRootQueueDiscOnDeviceByIndex (uint32_t index) const { NS_LOG_FUNCTION (this << index); return GetRootQueueDiscOnDevice (m_node->GetDevice (index)); } However, the GetNDevices () was not correct: uint32_t TrafficControlLayer::GetNDevices (void) const { return m_netDevices.size(); } m_node->GetNDevices () must be returned instead (m_netDevices.size () is smaller than m_node->GetNDevices () when a queue disc is not installed on some netdevice, e.g., loopback). The attached patch fixes this bug. Also, it shows how the config system can be used in the traffic-control example.
(In reply to Stefano Avallone from comment #5) > Created attachment 2485 [details] > Fix for traffic control > > The second bug is in the traffic control. I believe the definition of the > accessor functions is correct: > > template <typename T, typename U, typename INDEX> > Ptr<const AttributeAccessor> > MakeObjectMapAccessor (Ptr<U> (T::*get)(INDEX) const, > INDEX (T::*getN)(void) const); I stand corrected; I did not read beyond the definition that the rest of ns-3 codebase is using for ObjectMapAccessor: template <typename T, typename U> Ptr<const AttributeAccessor> MakeObjectMapAccessor (U T::*memberVariable); but there is also the variant that you make use of, plus one that inverts the order of the two methods. > > In TrafficControlLayer, we have: > > Ptr<QueueDisc> GetRootQueueDiscOnDeviceByIndex (uint32_t index) const; > uint32_t GetNDevices (void) const; > > thus: > > T = TrafficControlLayer > U = QueueDisc > INDEX = uint32_t > > My idea was to have a map<Ptr<NetDevice>, NetDeviceInfo> (which is > TrafficControlLayer::m_netDevices) so that TrafficControlLayer::Send > (Ptr<NetDevice> device, Ptr<QueueDiscItem> item) can directly use device to > access the map and find the queue disc where the item must be enqueued. On > the other hand, I wanted to allow users to access the traced attributes of > queue discs by using the config system: > > Config::ConnectWithoutContext > ("/NodeList/1/$ns3::TrafficControlLayer/RootQueueDiscList/0/PacketsInQueue", > MakeCallback (&TcPacketsInQueueTrace)); > > Here, the user provides the id of a netdevice (its id in NodeList) and gets > the root queue disc installed on that device. This is done by means of the > RootQueueDiscList attribute, which therefore must be an object container > storing m_node->GetNDevices () items. The i-th item stores the root queue > disc installed on the i-th (as in NodeList) device. This is correctly done: > > Ptr<QueueDisc> > TrafficControlLayer::GetRootQueueDiscOnDeviceByIndex (uint32_t index) const > { > NS_LOG_FUNCTION (this << index); > return GetRootQueueDiscOnDevice (m_node->GetDevice (index)); > } > > However, the GetNDevices () was not correct: > > uint32_t > TrafficControlLayer::GetNDevices (void) const > { > return m_netDevices.size(); > } > > m_node->GetNDevices () must be returned instead (m_netDevices.size () is > smaller than m_node->GetNDevices () when a queue disc is not installed on > some netdevice, e.g., loopback). > > The attached patch fixes this bug. Also, it shows how the config system can > be used in the traffic-control example. Agree.
What about pushing the fix for the config-store attribute iterator? I do not know if anyone else can have a look at it (looks like there is no maintainer for the config-store module).
(In reply to Stefano Avallone from comment #7) > What about pushing the fix for the config-store attribute iterator? I do not > know if anyone else can have a look at it (looks like there is no maintainer > for the config-store module). Please push it; there is no maintainer for config-store other than the project itself (Mathieu said a while back that he did not want to maintain it further). I agree with the patch and the rationale for it.
Pushed the proposed fixes with changesets 12171 and 12172.