Project

General

Profile

Task #3940

Restructure src/util/

Added by Davide Pesavento over 2 years ago. Updated almost 2 years ago.

Status:
Closed
Priority:
Low
Assignee:
Category:
Utils
Target version:
Start date:
Due date:
% Done:

100%

Estimated time:
5.00 h

Description

The src/util/ subdir contains a bunch of unrelated classes and functions. I suggest re-organizing it as follows:

  • move dns, ethernet, network-monitor, and detail/network-monitor-* to a new src/net/ directory
    • network-monitor code size will increase considerably after #3353 and #3817
  • face-uri can stay, or it can move to either src/net/ or src/, given that it's a pretty central and fundamental concept
  • move crypto and digest to src/security/
    • hopefully we can also reduce duplication between them, see #3886
  • dummy-client-face can stay, or it can move to a new src/dummyfw/ subdir together with the dummy forwarder being developed in #3913
    • feel free to bikeshed the exact name of the subdir
  • move in-memory-storage-* to its own subdir
  • move signal-* to its own subdir
    • keep the convenience header signal.hpp in src/util/
  • monotonic_deadline_timer and sqlite3-statement feel like they should go into a detail/ subdir somewhere, unless they're already used outside the library

This leaves us with the following in src/util/: backports & backports-optional, concepts, config-file, (maybe dummy-client-face), (maybe face-uri), indented-stream, io, logger & logging, notification-stream & notification-subscriber, random, scheduler & scheduler-scoped-event-id, segment-fetcher, string-helper, time & time-{custom,unit-test}-clock.

History

#1 Updated by Junxiao Shi almost 2 years ago

  • Assignee set to Junxiao Shi
  • Target version set to v0.6
  • % Done changed from 0 to 30
  • Estimated time set to 5.00 h

https://gerrit.named-data.net/3979 moves network-related classes to src/net:

  • ndn::dns
  • ndn::util::ethernet, renamed to ndn::ethernet namespace
  • NetworkMonitor and related classes, now in ndn::net namespace
  • FaceUri, now in ndn namespace

No compatibility headers are provided, because v0.6 is expected to be breaking.

https://gerrit.named-data.net/3981 updates NFD codebase following the above change.
Prior to that, https://gerrit.named-data.net/3980 fixes code style problems in nfd-autoreg.cpp.

ndn-tools, repo-ng, and traffic-generator are unaffected https://travis-ci.org/yoursunny/ndn-cxx-breaks/builds/248309680.
NLSR has pinned to older ndn-cxx.
ChronoSync and ChronoChat are already broken prior to this change.

#2 Updated by Junxiao Shi almost 2 years ago

  • Status changed from New to In Progress

#3 Updated by Davide Pesavento almost 2 years ago

Junxiao Shi wrote:

ndn-tools, repo-ng, and traffic-generator are unaffected https://travis-ci.org/yoursunny/ndn-cxx-breaks/builds/248309680.

ndn-tools is affected, see https://travis-ci.org/named-data/ndn-tools/builds/248956624

Looks like ndn-cxx-breaks doesn't build with tests enabled.

#4 Updated by Junxiao Shi almost 2 years ago

ndn-tools is affected

https://gerrit.named-data.net/3984

Looks like ndn-cxx-breaks doesn't build with tests enabled.

ndn-tools does not have --with-tests on 11JUN2015, so ndn-cxx-breaks does not have it.
It's added now.

#5 Updated by Junxiao Shi almost 2 years ago

  • % Done changed from 30 to 40

https://gerrit.named-data.net/3986 moves CFReleaser to util/cf-releaser-osx.hpp and into ndn::util namespace. It was in security/tpm but is being used in NetworkMonitor::Impl on macOS platform.

#6 Updated by Junxiao Shi almost 2 years ago

https://gerrit.named-data.net/3990 moves util/signal-*.*.
My question is, should I change namespace from ndn::util::signal to ndn::signal? The util part seems unnecessary given everything is already in a signal namespace. ndn::time, for example, does not have util part.

#7 Updated by Alex Afanasyev almost 2 years ago

I'm ok either way, with a slight preference of keeping the "util" namespace to reduce semi-pointless refactoring in the dependent code.

#8 Updated by Davide Pesavento almost 2 years ago

I have a slight preference for removing util::, but we can keep type aliases in ndn::util to avoid breaking other projects (or vice versa, keep them in ndn::util and add aliases in ndn namespace).

#9 Updated by Junxiao Shi almost 2 years ago

I'm usually aggressive in refactoring, so I've adopted ndn::signal::* and ndn::Signal in https://gerrit.named-data.net/3990 patchset2.
The old identifiers are deprecated.

An unfortunate side-effect of deprecating ndn::util::Signal is that, if Signal is used in code that itself is under ndn::util, a false deprecated warning would appear. This affects ndn::util::DummyClientFace and ndn::util::NotificationSubscriber.
I've changed the usages in headers to ndn::Signal to avoid polluting the output in dependant projects, but left them as is for .cpp and test suites.

#10 Updated by Davide Pesavento almost 2 years ago

Junxiao Shi wrote:

The old identifiers are deprecated.

I don't think there's any need to deprecate them now. The maintenance cost is zero, and as Alex said it would just trigger an avalanche of pointless changes in dependent projects. You may use doxygen to document the old aliases as deprecated though.

#11 Updated by Junxiao Shi almost 2 years ago

I disagree with having 3 names for the same Signal type. Thus, I'll revert to ndn::util::Signal.

You may use doxygen to document the old aliases as deprecated though.

This is useless. Based on past experience, dependant projects will NEVER notice until there's a warning/error, regardless of how many "deprecation notice" emails I send to ndn-lib.

#12 Updated by Junxiao Shi almost 2 years ago

move in-memory-storage-* to its own subdir

This one is going to get ugly. The class name is InMemoryStorageLru. Changing it to in_memory_storage::Lru is bad because the class name itself Lru does not indicate it is-a InMemoryStorage but feels like a tag.
Without changing the class name, rule 3.2 forces the header file to be named in-memory-storage-lru.hpp, and the include line becomes #include <ndn-cxx/util/in-memory-storage/in-memory-storage-lru.hpp> which repeats "in-memory-storage" twice.

#13 Updated by Davide Pesavento almost 2 years ago

Junxiao Shi wrote:

move in-memory-storage-* to its own subdir

This one is going to get ugly. The class name is InMemoryStorageLru. Changing it to in_memory_storage::Lru is bad because the class name itself Lru does not indicate it is-a InMemoryStorage but feels like a tag.

Who said anything about changing the class name?

Without changing the class name, rule 3.2 forces the header file to be named in-memory-storage-lru.hpp, and the include line becomes #include <ndn-cxx/util/in-memory-storage/in-memory-storage-lru.hpp> which repeats "in-memory-storage" twice.

Self-inflicted problems. Besides, "in-memory-storage" is kind of long for a directory name, I suggest something shorter like "ims" or "memstore" or similar...

#14 Updated by Junxiao Shi almost 2 years ago

  • % Done changed from 40 to 50

https://gerrit.named-data.net/4037 MonotonicDeadlineTimer
Note that MonotonicDeadlineTimer is now a subclass of boost::asio::basic_deadline_timer<time::steady_clock> instead of a typedef, because scheduler.hpp needs to forward-declare it.

#15 Updated by Junxiao Shi almost 2 years ago

  • % Done changed from 50 to 70

#16 Updated by Junxiao Shi almost 2 years ago

why not moving it up one level to src/ims ?

I can, but do I also change the namespace? Should types be named like ndn::ims::InMemoryStorageLru, or even ndn::InMemoryStorageLru?

#17 Updated by Davide Pesavento almost 2 years ago

An ims namespace doesn't seem to be necessary, but I'm fine either way.

#18 Updated by Junxiao Shi almost 2 years ago

  • Status changed from In Progress to Resolved

I do not plan to do the following:

dummy-client-face can move to a new src/dummyfw/ subdir together with the dummy forwarder being developed in #3913

This can be performed after #3913 is done.

sqlite3-statement feel like they should go into a detail/ subdir somewhere, unless they're already used outside the library

Although sqlite3-statement is unused outside ndn-cxx, it shall be useful to simplify repo-ng and ndns where SQLite3 database is used.

This issue will close after 24 hours if there's no objection.

#19 Updated by Davide Pesavento almost 2 years ago

Junxiao Shi wrote:

dummy-client-face can move to a new src/dummyfw/ subdir together with the dummy forwarder being developed in #3913

This can be performed after #3913 is done.

sqlite3-statement feel like they should go into a detail/ subdir somewhere, unless they're already used outside the library

Although sqlite3-statement is unused outside ndn-cxx, it shall be useful to simplify repo-ng and ndns where SQLite3 database is used.

Fair enough.

What about:

move crypto and digest to src/security/

crypto.hpp is deprecated and will be removed soon, so there's no point in moving it around. digest.hpp should be moved.

#20 Updated by Davide Pesavento almost 2 years ago

Btw, the pib tool in ndn-tools uses in-memory-storage, so it's now broken (although it's already not built by default IIRC).

#21 Updated by Junxiao Shi almost 2 years ago

  • % Done changed from 70 to 100

crypto.hpp is deprecated and will be removed soon, so there's no point in moving it around. digest.hpp should be moved.

I disagree with placing Sha256 class into security/ directory.

  • SHA256 algorithm itself is not specific to NDN security (signing and verifications). It is a general utility. Its use case out of security includes data disambiguation via ImplicitSha256DigestComponent.
  • Placing a SHA256 algorithm helper together with signature types that involve SHA256 such as signature-sha256-with-rsa.hpp would be confusing.

https://gerrit.named-data.net/4061 renames util/digest.hpp to util/sha256.hpp to match the enclosed class name, as required by rule 2.1. Deprecated util/crypto.hpp is removed in the same commit.

No compatibility headers are provided, because v0.6 is expected to be breaking.

https://gerrit.named-data.net/4062 updates NFD codebase following the above change.

ndn-tools (excluding disabled PIB) and traffic-generator are unaffected https://travis-ci.org/yoursunny/ndn-cxx-breaks/builds/256747285.
NLSR has pinned to older ndn-cxx.
repo-ng, ndns, ChronoSync, and ChronoChat are already broken prior to this change.

#22 Updated by Davide Pesavento almost 2 years ago

Junxiao Shi wrote:

  • SHA256 algorithm itself is not specific to NDN security (signing and verifications). It is a general utility. Its use case out of security includes data disambiguation via ImplicitSha256DigestComponent.

Several things in src/security/ are not specific to NDN security... e.g. the whole transform framework... So this is not really a valid point.

  • Placing a SHA256 algorithm helper together with signature types that involve SHA256 such as signature-sha256-with-rsa.hpp would be confusing.

Why would it be confusing? It's no more confusing than the difference between the two algorithms, or the difference between RSA and ECDSA, or virtually anything security-related. If one doesn't know the difference between a signature and a digest, they shouldn't be touching security-related things altogether, everything would look confusing to them. So this point is also invalid.

ndn-tools (excluding disabled PIB) and traffic-generator are unaffected https://travis-ci.org/yoursunny/ndn-cxx-breaks/builds/256747285.

You can't ignore the pib tool just because it's not built by default.

#23 Updated by Davide Pesavento almost 2 years ago

Davide Pesavento wrote:

Junxiao Shi wrote:

  • Placing a SHA256 algorithm helper together with signature types that involve SHA256 such as signature-sha256-with-rsa.hpp would be confusing.

Why would it be confusing? It's no more confusing than the difference between the two algorithms, or the difference between RSA and ECDSA, or virtually anything security-related. If one doesn't know the difference between a signature and a digest, they shouldn't be touching security-related things altogether, everything would look confusing to them. So this point is also invalid.

Something that can cause confusion is the difference between security/digest-sha256.hpp and util/sha256.hpp, and this confusion can arise regardless of the subdir in which the headers reside.

#24 Updated by Junxiao Shi almost 2 years ago

20170724 NFD call decides that Sha256 can stay in util/.

#25 Updated by Alex Afanasyev almost 2 years ago

Just for the record. I have no objections of renaming to sha256.hpp and keeping it in util.

#26 Updated by Junxiao Shi almost 2 years ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF