Bug #3259
closed
Potential design flaw in Transport::beforeChangePersistency
Added by Davide Pesavento over 8 years ago.
Updated over 8 years ago.
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.
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.
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::NONE value (numeric code should be -1, so that existing codes do not change)
Transport constructor sets persistency to NONE- Subclass constructor should initialize persistency to a meaningful value.
- Subclass
beforeChangePersistency should 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.
20151015 conference call approves note-2.
- Status changed from New to In Progress
- Estimated time set to 2.00 h
- % 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.
Junxiao Shi wrote:
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.
What's wrong with static_cast<uint64_t>?
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.
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.
Requiring static_cast in 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.
- Status changed from In Progress to Code review
- % Done changed from 50 to 100
- Status changed from Code review to Closed
Also available in: Atom
PDF
Updated by 
Updated by