NFD » ndn-cxx

Design issue:

In commit:adfbf23006d269c379f9f4eff1c0d1c4d72ee7ab, ndn::security::ValidityPeriod is a subclass of Block.
This is harmful, because:

• TLV-VALUE of ValidityPeriod has a certain internal structure.
• By public-inheriting from Block, the caller is able to modify the TLV-VALUE using Block APIs, without going through ValidityPeriod APIs.

The result is, a ValidityPeriod instance is not guaranteed to represent a valid ValidityPeriod abstraction.

A preceding example of having an abstraction type inheriting from Block is name::Component.
However, there's a fundamental difference in that case:

• TLV-VALUE of NameComponent is unstructured.
• Any modification to the NameComponent's TLV-VALUE using Block APIs still results in a valid NameComponent.

Inheriting from Block allows simplified and generalized implementation of SignatureInfo block without requiring any additional memory overhead.

If somebody wants to do anything bad with the abstraction, it can do it anyways. We are developing for normal use cases, not something that someone can do to make something bad.

In the implementation I specifically added invalid value handling in the only method that interprets the value (getPeriod). Even if somebody screws up with the content of the value, there is no unintended memory access and only exception will be generated.

Design problem in commit:8673bad855a5b0c4a240063806ad068515b3218d :

ValidityPeriod public API is using time_point<system_clock, nanoseconds> to represent NotBefore and NotAfter fields.
This type indicates a precision of nanoseconds.

However, ValidityPeriod is only capable to storing NotBefore and NotAfter fields at seconds precision.

The Chrono library (std::chrono or boost::chrono) is all about being explicit about type (duration vs time_point) and precision (nanoseconds vs seconds, and others).

It's semantically wrong to indicate a precision of nanoseconds while the real precision is seconds.

The public API shall use time_point<system_clock, seconds> type to indicate a correct precision.

Alternatively, use time_t type which has the same clock and precision, so caller doesn't have to do time_point_casts.

Junxiao, please stop making unreasonable suggestions. I agreed with many others, but will not agree with this one.

There is no problem with the implementation. Whatever ValidityPeriod internally uses it is internal business for the ValidityPeriod. Public interface is in terms of "standard" TimePoint that is used throughout the application.

Reply to note-9:

note-8 is not an implementation problem, but a semantics problem in the API design.

Chrono library is all about being explicit about type and precision.

In API design, it's important to avoid surprises.

It would be surprising when the getter returns a different value than what's passed to the setter due to loss of precision.

This can be avoided by using time_point<system_clock, seconds> or time_t to indicate the correct precision.

I think it's misleading to have an API that handles nanoseconds while the underlying data type has a precision of one second. It's like having getFoo/setFoo() methods where a double is being passed, but the corresponding member field is a float. So I tend to agree with Junxiao here.

I am not changing the code. We are talking about some trivial not-even-correctness things, which would result in significant increase in complexity when using ValidityPeriod abstraction---everybody will have to add unnecessary boost::time_point_cast<time::seconds>(...) in order to actually use the constructor.

Reply to note-12:

This is a correctness issue.
Using the wrong precision is semantically wrong.

An API that leads to a surprise (see note-10) is bad.

If you want to avoid time_point_cast, design the API to use time_t type, which has the same meaning as time_point<system_clock, seconds>.