NFD

Should I wait for Eric finish his task?

Junxiao Shi wrote:

The assignee may decide whether to have a DatagramTransport template that is shared between UdpTransport and future datagram-based transports such as UnixSeqPacketTransport (#1672), or to implement them separately.

UnixSeqPacketTransport is not datagram-based (as in SOCK_DGRAM-based). It is based on the SOCK_SEQPACKET socket type. In any case I wouldn't worry about sharing code between the two, as it is not clear yet whether a UnixSeqPacketTransport will ever be implemented.

Should I wait for Eric finish his task?

Implementation cannot start until #3088 is merged.

However, you may start reading existing UdpFace code and Transport API.

Should I submit UdpTransport without LinkService and Transport? And should I create a new unit-test for transport, or just modify the original udp.t.cpp?

Thanks!

Answer to note-14:

Should I submit UdpTransport without LinkService and Transport?

The commit to upload should have #3088 as the parent commit.

(I have explained how to do this in a conference call with the assignee)

And should I create a new unit-test for transport, or just modify the original udp.t.cpp?

I'm looking through udp.t.cpp and it appears that existing test cases should continue to work, unchanged, with LpFaceWrapper(LpFace(GenericLinkService+UdpTransport)).

I'm unsure about the necessity of new test cases for UdpTransport.

I may separate MultipleUdpTransport since UdpTransport and MultipleUdpTransport have different doSend(): async_send and async_send_to.

So I will keep datagramTransport. And it will minimize the damage to original process.

One more question:

Should I put UdpChannal into namespace Face?

Yukai Tu wrote:

I may separate MultipleUdpTransport since UdpTransport and MultipleUdpTransport have different doSend(): async_send and async_send_to.

What is MultipleUdp? do you mean Multicast?

The difference between async_send and async_send_to isn't really a difference for UDP, you can use async_send_to in both cases if you want.

Yukai Tu wrote:

Should I put UdpChannal into namespace Face?

I think in another ticket I've already argued against excessive namespacing in NFD, so I would disagree with moving the whole faces subsystem into namespace face.

Davide Pesavento wrote:

The difference between async_send and async_send_to isn't really a difference for UDP, you can use async_send_to in both cases if you want.

Yes, except this difference, it seems that combining two transport is accetable. The original design uses async_send in UdpFace since it can avoid reimplementing dst/port-based dispatch manually(http://redmine.named-data.net/issues/2989, note-37, Alex), I prefer keep these two transport separated.

"dstport-based dispatch" is relevant to receiving only.

async_send_to with the correct remote endpoint should have the same effect as async_send.

No, this issue should not change namespace of UdpChannel or UdpFactory.

Yukai and I had a conference call about this issue. Summary:

• It's necessary to have separate Transports for unicast and multicast, because multicast needs two separate sockets for sending and receiving. The classes should be named UnicastUdpTransport and MulticastUdpTransport, because there's no inheritance relation between these two. Both can inherit from DatagramTransport class template.
• In UdpChannel::handleNewPeer, face->receiveDatagram(m_inputBuffer, nBytesReceived, error) should be changed to calling a method on UnicastUdpTransport. This requires LpFaceWrapper to have a getter that returns a regular pointer to the underlying LpFace; saving a pointer to the original UnicastUdpTransport or LpFace is ineffective because they have been moved away and there's no guarantee the saved pointer is still valid.
• Idle detection remains in UnicastUdpTransport. Any incoming packet that can be parsed as valid TLV block indicates "recent usage".
• If the face should be closed due to idle, UnicastUdpTransport can invoke this->close();.
• DatagramTransport::doClose should close its socket.
• MulticastUdpTransport::doClose should invoke DatagramTransport::doClose then close its send socket. This requires Transport::doClose to be changed from private to protected.

I have compiled the code successfully. When I run unit-tests, there are some unit-tests need Counter.

Reply to note-24:

The existing test suite should work just fine with the counters on LpFaceWrapper.

The counters on LpFace Transport aren't incremented, and aren't accessible from existing test suite.

In any case, upload your patchset to Gerrit, so others can have a look.

In UdpChannel::handleNewPeer, I use LpFaceWrapper::getLpFace()->getTransport, then static_cast it to the UnicastTransport, in which we use receiveDatagram(). The original process will get MutableCounters in datagramFace, but now we don't. That's the point of the failure of the unit-tests.

Any way, I will submit my code for review.

Reply to note-26:

I can see the problem comes from: DatagramFace increments link layer byte counter, but DatagramTransport appears not to increment those counters.
DatagramTransport can increment the counter after #3177, but those counters are visible within LpFace and are not exposed in LpFaceWrapper.

I would say, mark these assertions as expected failures for now, until either #3172 or when we find another solution.

Junxiao Shi wrote:

"dstport-based dispatch" is relevant to receiving only.

async_send_to with the correct remote endpoint should have the same effect as async_send.

yep, exactly.

In original process, we sent out the interest.wireEncode(), now we send out lpPacket.wireEncode(). It seems that theirs' bytes are different.

Reply to note-29:

They won't be different after #2930 note-18 is implemented.

Two problems:

1. In unit-tests, FaceHistory will record the face's failure. In LpFaceWrapper's close method, it dosen't call the fail(), which causes FaceHistory can't record the face failure. I add one line in LpFaceWrapper::close(): this->fail("Face closed"). I think that should be added to #3088.

2. The same as above, this time is when UdpTransport out of time, it will call Transport::close(). But in Transport, we can't get Face::fail(), so I commend two line in udp.t.cpp, waiting for the future design.

I just submit the UdpTransport on gerrit which can build successfully and trought all unit-tests. I think it can be used by other person who will develop based on it. If anything gose well, I can begin UnixStreamTransport.

Reply to note-31:

LpFaceWrapper::close is not the right place to signal (old)Face::onFail. It should hook to LpFace::afterStateChange signal.

I've added the correct solution to commit:191178a0cfb12d9111df782f0e1012cc92dda0df.

I may need some help about cherry-pick. Since I use command git fetch ssh://[email protected]:29418/NFD refs/changes/23/2423/24 && git cherry-pick FETCH_HEAD, after that I solve the conflict manually, I can't amend my commit.

Hi Yukai,

You need to have different order. As a general rule, you don't cherry-pick other commits on top of your commit. You cherry pick or rebase your commit on top of others.

The following commands will have equivalent effect:

# I'm assuming your commit is ab872cda0de6a54c76d2f9a71d6a572ac5fcb568

# if you are not yet checkout your commit
git checkout -b udp-transport ab872cda0de6a54c76d2f9a71d6a572ac5fcb568

# Retrieve existing code, which will be a new base for your commit
git fetch http://gerrit.named-data.net/NFD refs/changes/23/2423/24

# Request interactive rebase on top of just retrieved code
git rebase -i FETCH_HEAD

# In the presented view, select which commits you want to apply, save, and quit
# resolve conflicts using
git mergetool

# Finish up with
git rebase --continue

Same thing, but explicitly with cherry-pick

# Retrieve existing code, which will be a new base for your commit
git fetch http://gerrit.named-data.net/NFD refs/changes/23/2423/24

# Create a new branch for the code based on just fetched code
git checkout -b udp-transport FETCH_HEAD

# Cherry-pick your code
git cherry-pick ab872cda0de6a54c76d2f9a71d6a572ac5fcb568

# Resolve conflicts
git mergetool

# Finish up
git cherry-pick --continue

#or just 
git commit

Hi Alex:

Thanks for your help. I had found my commit id.

When I finished all these steps, I commit my code. But it said that Eric's commit's author email doesn't match my email account.