NFD

I have finished implementing the added Signals to rib.cpp, and added the struct definition to rib.hpp. I am unsure what you mean by "separate commits." Do you intend for me to push this patch to gerrit, and then to move on to step 2?

Do you intend for me to push this patch to gerrit, and then to move on to step 2?

Yes, you should upload RIB signals commit to Gerrit. Subsequent steps should be performed in separate commits.

I have been struggling with code review and unit tests for a while now (mostly as a result of general unfamiliarity with the NFD codebase and its design). However today I have had some success that prompts a question:
To make the unit test succeed it was required that I fully specify all of the parameters in the ControlParameters used in the advertise function. I don't understand why I've had to do this, though, because the RibMgmt documentation says that these fields should have defaults applied to them if they are absent. Further, I wasn't able to find a line outside of the unit tests where the functions "applyDefaultsToRequest" is used, which seems to me like they aren't used anywhere at all.

Can someone explain to me what's going on?

I wasn't able to find a line outside of the unit tests where the functions "applyDefaultsToRequest" is used

If you grep the entire codebase, you should be able to find the only invocation.

I've managed to muddy the waters for myself again. I'm currently implementing the methods for Readvertise, but I've noticed that ReadvertisedRoute was designed with only a getter for a const version of the vector. The header design is unclear whether on afterAddRoute() a new, distinct ReadvertisedRoute should be created and added to the Readvertise instance, or if the existing one by prefix should be updated with more RibRouteRefs.

It is unclear whether RibRouteRefs for a common name should be kept together in one ReadvertisedRoute instance. If this is the case, then the ReadvertisedRoute needs to be extended to allow for adding a RibRouteRef to the member vector. If that is not the case, then why was it designed with a vector of RibRouteRefs in the first place and not just a singular instance? To be clear, in the case that the are meant to be aggregated, then on .advertise() and .withdraw(), each RibRouteRef in the vector must be used, correct?

Some clarification would be appreciated.

Since I have received no comments on note-9, I have assumed that the ReadvertisedRoute's list of ribRouteRefs should be mutable in some way, and have added mutator methods to it.

I don't understand exactly what is meant by "destination available." Is this some functionality available elsewhere, or is it implemented by each derived ReadvertiseDestination class? I cannot proceed until I understand this, so some help would be much appreciated.

ReadvertisedRoute was designed with only a getter for a const version of the vector<RibRouteRef>

https://gist.github.com/yoursunny/bcee67893134d7d49c0868f5a8b9ed41/2e3a6e24042be5f02984e90815fe34567a9f8ace#file-readvertise-hpp-L39-L42
ReadvertisedRoute::getRibRoutes() returns a mutable reference of the vector, so that you can modify the returned reference directly.

I've been trying to write the unit tests for the NLSR component that listens for Readvertise commands, however I am having some difficulty using the DummyClientFace class to mock receiving interests through the dispatcher. I have posted my code snippets here:

https://gist.github.com/gorgonical/2ab5eff58c81a60e45dbaee99e087298

I understand that this is rather vague, but I don't really even know what I'm doing wrong, much less how to resolve it. At this point, none of my dispatcher callbacks are being called, as verified by GDB, but the ControlCommand handlers appear to be registered successfully in the dispatcher. This suggests to me that I am misunderstanding something about how to simulate Interests on this Face or about how the dispatcher works. I've tried emulating the ndn-cxx and nfd tests that involve the dispatcher, but that hasn't worked either. Any tips or discussion about how to correctly mock up what I'm doing would be very helpful and much appreciated.

I need a little help understanding how to effectively use this boost::multi_index_container. First, I can't use std::set because it only allows searching based on element identity. This doesn't mimic the previous data structure very well. This multi_index_container is working well, except all of the elements in the container are const. This means that the object has to be copied (or moved) from the container, modified, then placed back into the container.

I can do that, but I have to implement a lot of move mechanics and heavily refactor the code. Is that really worth the cost saving of storing two Names? Names are essentially fancy strings, and I wonder about the tradeoff in simplicity for the small improvement in memory efficiency.

If this refactor is insisted upon, I would appreciate some help about how to use this container effectively.

I have uploaded some code snippets to this gist: https://gist.github.com/gorgonical/3308d405ccbbc3279c7576e7c7678d7d

Any comments would be appreciated.

This multi_index_container is working well, except all of the elements in the container are const. This means that the object has to be copied (or moved) from the container, modified, then placed back into the container.

Have you looked at the modify method on an index type?

modify will work for this situation, however, I have a problem in that the const semantics of the multi_index_container complicated things significantly. I'm running into issues using the container effectively, and in the interest of time, would it be an acceptable alternative to use the string representation the ReadvertisedRoute's prefix as the key in a map? I think that I could work out the issues given enough time, but I don't see it as being worth it; I'm unfamiliar with the construct and the const semantics ask for a large refactor of the patch. However, I see that Name's string representations (should) be unique, so that should be a cheap key.

would it be an acceptable alternative to use the string representation the ReadvertisedRoute's prefix as the key in a map?

No, this is unacceptable.

The current patch set fails on the Ubuntu 14.04 slave because the version of the Boost library installed there (1.54) does not have the emplace function.
I see two options:

1. Refactor the patch set to use insert, which I don't think would be too much hassle.
2. Update the libraries on that host.

The only caveat is that insert is slightly less efficient, because the move constructor would be used to move the objects.

The current patch set fails on the Ubuntu 14.04 slave because the version of the Boost library installed there (1.54) does not have the emplace function.

1. Update the libraries on that host.

You can't. See platform policy.

The only caveat is that insert is slightly less efficient, because the move constructor would be used to move the objects.

To avoid the performance penalty:

#if BOOST_VERSION >= 105500
  m_index.emplace(..);
#else
  m_index.insert(..);
#endif

Regarding 02-16-2017's NFD call, I claimed that the Readvertise patch would be 100% feature-complete when this current patch on gerrit cleared, and Junxiao agreed with me. However, I do not think this is true. Nowhere in the patches is a Readvertise instance created that would actually connect to the RIB signals added earlier. This is not a large patch, but it does need to come along.

I propose adding the Readvertise instance to the RibManager. It could be added to the RIB itself, but it would require refactoring the RIB some; NfdRibReadvertiseDestination requires a controller instance, which the RIB does not have, but the RibManager does. Additionally, the AutoPrefixPropagator instance is already located in the RibManager. Further, the design of Readvertise requires that the instance not be created if readvertising is not desired. This is easiest to accomplish when the RibManager processes the config file, if the RibManager had for example a unique_ptr to its Readvertise instance.

Readvertise should be owned by nfd::rib::Service.

I propose adding the Readvertise instance to the RibManager.

No. RibManager should only be responsible for processing incoming commands.

the AutoPrefixPropagator instance is already located in the RibManager.

That was a mistake. It should be moved into nfd::rib::Service.

Further, the design of Readvertise requires that the instance not be created if readvertising is not desired. This is easiest to accomplish when the RibManager processes the config file, if the RibManager had for example a unique_ptr to its Readvertise instance.

rib config section should be processed in nfd::rib::Service.

You may add two more commits after the commit adding Readvertise itself:

• rib: process rib config section in rib::Service (this commit also moves AutoPrefixPropagator)
• rib: enable readvertise into NLSR

Finally, the config key for readvertising into NLSR should be rib.readvertise_nlsr. It can be a simple yes/no or a section like auto_prefix_propagate.

I've pushed a first draft of the Readvertise documentation to the Dev guide repo. Let me know what you think of the additions. I've placed the section just above the AutoPrefixPropagator section.

If no one has any comments, I can close this issue then?