NFD

Junxiao Shi wrote:

https://gerrit.named-data.net/3513 splits configuration processing to FaceSystem class.

I don't like FaceSystem too much as a class name, as I commented on patch set 2. I don't have a better name unfortunately, maybe FaceSystemConfigurator?

No. FaceSystemConfigurator is too long, and it's inappropriate for a "configurator" to own any ProtocolFactory objects. Instead, it's very logical to let FaceSystem own face system's high level ProtocolFactory objects.

There's a concern about NicManager::enableNetworkMonitor causing problem for simulator, and a request for disabling the effect of this function via #ifdef. But the exact purpose I introduced this separate function rather than initializing creating NetworkMonitor in NicManager is to support simulators: This function eventually will be exposed to the initialization procedure in Nfd class; when NFD is running in the simulator, the initialization code (such as ndnSIM's L3Protocol) should not invoke this function. I don't think a #ifdef is necessary.

Junxiao Shi wrote:

https://gerrit.named-data.net/3517 splits config/NIC-triggered face creation/destroying to NicManager.

I disagree with this design. NicManager is still responsible for a bunch of random unrelated things: reacting to network AND config file changes, which are completely independent from one another. You simply moved this mess from FaceSystem to NicManager, but the new "design" is not any simpler. In fact, the initial creation of multicast faces is now split between the two classes, making the code even harder to read. The overall result is worse than the status quo.

Reply to note-8:

The main responsibility of NicManager is to respond to NIC changes detected by NetworkMonitor. This responsibility is not "completely independent" from configuration file, because every time there is a NIC change, it is necessary to reevaluate a subset of the configuration. For example, if a new NIC is added, NicManager should check each whitelist+blacklist to see whether multicast faces and NIC-associated permanent faces should be created on the NIC. Therefore, NicManager needs to remember part of the configuration.
I decide to let FaceSystem parse the configuration rather than passing ConfigSection to NicManager because some sections such as face_system.udp contains both fields for unicast channels and fields for multicast faces. Also, since NIC changes are expected to occur more often than configuration reloads, the configuration is stored as structs rather than raw ConfigSection, so that they don't have to be re-parsed every time; this design is consistent with NLSR.

I'm thinking about a different design:
Instead of having FaceSystem parse the configuration file, we delegate each config section to the ProtocolFactory. For example, when FaceSystem encounters face_system.udp section, it creates a UdpFactory instance, and let UdpFactory parse that section and create channels and multicast faces accordingly. NetworkMonitor is created by FaceSystem, and shared to ProtocolFactory instances via a shared pointer. Common procedures can be added to ProtocolFactory base class if needed.
For NIC-associated permanent faces, a separate component parses the nicfaces section, and gives relevant rule sections to UdpFactory and other ProtocolFactory instances (if we decide to support tcp4).

note-10 idea sounds much better.

Don't forget to update NFD's devguide.

https://gerrit.named-data.net/3535 patchset8 ensures ProtocolFactory::processConfig is invoked even if the relevant subsection has been deleted.

https://gerrit.named-data.net/3539 implements note-10 design in UnixStreamFactory.
patchset1 has a small behavior change: when Unix sockets are disabled in Waf script, the error message would be "Unrecognized option face_system.unix" instead of an explanation that Unix sockets were disabled during compilation.

https://gerrit.named-data.net/3551 patchset2 is a preview revision of EthernetFactory implemention, whose structure will be applied to UdpFactory as well.
Notably, processConfig stores multicast configuration inside EthernetFactory, so that when NetworkMonitor reports a NIC addition, EthernetFactory can decide whether to create a multicast face on the new NIC without requiring a configuration reload.
createMulticastFace is made private, to ensure all multicast faces are created from the configuration and therefore all of them can be destroyed if the group address is changed in the configuration.

patchset1 has a small behavior change: when Unix sockets are disabled in Waf script, the error message would be "Unrecognized option face_system.unix" instead of an explanation that Unix sockets were disabled during compilation.

I'm not happy with the above, but I can live with it.

It's possible to restore the old error message by registering a dummy factory on "unix".
I can consider this, but only after all factories are converted.

I'm not sure I agree with this change. Yes, we have a very restricted use of websocket channel right now, but I would exclude a possibility in the future implementation to connect to other NFD (read = in-browser NFD) over websocket channel.

What type of conflict for IPv4/6 websocket channels you're referring to?

Reply to note-20:

Restricting to only one channel does not conflict with the possibility of initiating outgoing WebSocket connections. When that is needed, we can create a non-listening channel.

What type of conflict for IPv4/6 websocket channels you're referring to?

One may create an IPv4 channel at ws://192.0.2.1:9696 and an IPv6 channel at ws:/[::ffff:192.0.2.1]:9696. They are the same endpoint.

I also disagree with the channel-related changes in note-19. The motivations given are bogus. I could say the same thing for TCP and UDP channels, at the moment they are always bound to a hardcoded 0.0.0.0/[::], and we don't have a way to change that or create other channels.

One may create an IPv4 channel at ws://192.0.2.1:9696 and an IPv6 channel at ws:/[::ffff:192.0.2.1]:9696. They are the same endpoint.

The same can happen for TCP and UDP, so this is not a valid reason to treat Websocket channels differently. In the above scenario, I would expect an error to be raised, and that is the proper response if one creates such a configuration. In other words, I don't see any problem here.

https://gerrit.named-data.net/3623 makes ProtocolFactory::processConfig pure virtual.

Junxiao Shi wrote:

NFD devguide nfd-docs:commit:f9e61e649f014defda8e0346b2feaf345214e7d3

I did a few minor adjustments, but generally LGTM. I also commented out the "NIC-associated permanent faces" subsection you added, I think it's way too early to publicly document that functionality, since it's largely unimplemented.

This task is not done yet though.

1. You promised Alex that you were going to add #ifdefs for each protocol factory type, including TCP and UDP, to reduce the amount of patches that ndnSIM devs need to apply
2. See note-18

Junxiao Shi wrote:

add #ifdefs for each protocol factory type, including TCP and UDP, to reduce the amount of patches that ndnSIM devs need to apply

This is no longer needed. ndnSIM only needs to disable compilation of tcp-.cpp and udp-.cpp, just like it does for other ProtocolFactory types.

It is my understanding that Alex wanted to do this via build-time configuration macros (e.g. HAVE_TCP, HAVE_UDP). I'll let him comment on whether the current way of excluding files is also acceptable for ndnSIM or not.