Bug #3259
closed
Potential design flaw in Transport::beforeChangePersistency
100%
Description
UnicastUdpTransport supports persistency transitions only in the direction on-demand -> persistent -> permanent. It makes sense to override Transport::beforeChangePersistency to throw when an illegal transition is attempted.
However, this prevents creating an on-demand UDP face, because Transport constructor sets the initial persistency to persistent, and when UnicastUdpTransport constructor tries to set it back to on-demand, an exception will be thrown from beforeChangePersistency.
Updated by Davide Pesavento over 8 years ago
Actually, it might work in practice, because virtual calls from constructors become non-virtual (i.e. the "current class" version is called), so setPersistency would end up calling the base class version of beforeChangePersistency, which does nothing.
Still, it seems rather fragile and inconsistent to rely on this behavior.
Updated by Junxiao Shi over 8 years ago
This problem is caused by Transport base class constructor setting persistency to persistent as the default.
Since subclass must not rely on these defaults, the solution shall be:
- Introduce a
FacePersistency::NONEvalue (numeric code should be -1, so that existing codes do not change) Transportconstructor sets persistency to NONE- Subclass constructor should initialize persistency to a meaningful value.
- Subclass
beforeChangePersistencyshould permit any change from NONE to a supported value.
Similar NONE values should be introduced to FaceScope and LinkType types.
In addition, all three enums should be changed to enum class for stronger type checking; old names such as FACE_PERSISTENCY_ON_DEMAND can be kept as deprecated aliases.
Updated by Junxiao Shi over 8 years ago
20151015 conference call approves note-2.
Updated by Junxiao Shi over 8 years ago
- Status changed from New to In Progress
- Estimated time set to 2.00 h
Updated by Junxiao Shi over 8 years ago
- % Done changed from 0 to 50
http://gerrit.named-data.net/2518
I tried the change to enum class but there are obstacles: FaceScope FacePersistency LinkType are used as TLV-VALUEs, but encoding-helpers are not ready to accept enum class.
Snippet from nfd-face-status.cpp:
totalLength += prependNonNegativeIntegerBlock(encoder,
tlv::nfd::FaceScope, m_faceScope);
where prependNonNegativeIntegerBlock is declared as:
template<Tag TAG>
size_t
prependNonNegativeIntegerBlock(EncodingImpl<TAG>& encoder, uint32_t type, uint64_t value);
Implicit conversion from an enum class to uint64_t is disallowed.
Changing to enum class would require encoding APIs such as prependNonNegativeIntegerBlock to be extended and accept enum class as the TLV-VALUE field.
Updated by Davide Pesavento over 8 years ago
Junxiao Shi wrote:
Implicit conversion from an
enum classtouint64_tis disallowed.
Changing toenum classwould require encoding APIs such asprependNonNegativeIntegerBlockto be extended and acceptenum classas the TLV-VALUE field.
What's wrong with static_cast<uint64_t>?
Updated by Junxiao Shi over 8 years ago
What's wrong with
static_cast<uint64_t>?
The condition of note-4 approval is for this to be a non-breaking change.
Requiring static_cast in calling code would be a breaking change, because code similar to note-6 snippet may exist out of ndn-cxx.
Updated by Davide Pesavento over 8 years ago
Junxiao Shi wrote:
What's wrong with
static_cast<uint64_t>?The condition of note-4 approval is for this to be a non-breaking change.
Requiringstatic_castin calling code would be a breaking change, because code similar to note-6 snippet may exist out of ndn-cxx.
Sure, but I don't think there is a lot of code out there that does that. And the fix is trivial.
Alternatively, adding wrappers/overloads for prependNonNegativeIntegerBlock sounds acceptable too. Maybe we can immediately mark them as deprecated.
Updated by Junxiao Shi over 8 years ago
- Status changed from In Progress to Code review
- % Done changed from 50 to 100
Updated by Junxiao Shi over 8 years ago
- Status changed from Code review to Closed