Bug #3259: Potential design flaw in Transport::beforeChangePersistency - NFD - NDN project issue tracking system

Actions

Copy link

Bug #3259

closed

Potential design flaw in Transport::beforeChangePersistency

Added by Davide Pesavento about 10 years ago. Updated almost 10 years ago.

Status:

Closed

Priority:

Normal

Assignee:

Junxiao Shi

Category:

Faces

Target version:

v0.4

Start date:

Due date:

% Done:

100%

Estimated time:

2.00 h

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.

Actions

Copy link

Updated by Davide Pesavento about 10 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.

Actions

Copy link

Updated by Junxiao Shi about 10 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::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.

Actions

Copy link

Updated by Davide Pesavento about 10 years ago

Yes.

Actions

Copy link

Updated by Junxiao Shi almost 10 years ago

20151015 conference call approves note-2.

Actions

Copy link

Updated by Junxiao Shi almost 10 years ago

Status changed from New to In Progress
Estimated time set to 2.00 h

Actions

Copy link

Updated by Junxiao Shi almost 10 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.

Actions

Copy link

Updated by Davide Pesavento almost 10 years ago

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>?

Actions

Copy link

Updated by Junxiao Shi almost 10 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.

Actions

Copy link

Updated by Davide Pesavento almost 10 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.

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.

Actions

Copy link

#10