remove iot tlv from ccn-lite #222

mfrey · 2018-03-28T09:53:42Z

This PR removes support for the IOT TLV format (and addresses #212). After the review and approval the commits need to be squashed before merging. I've basically tried to split the whole removal process into small pieces, so reverting (if necessary) should be easier.

blacksheeep

I did not go through the changes line by line.
What I can say:

scripted demo scenarios from Tutorial in Berlin are good
scripted NFN demo is good.
NFN demos uses:
ICN data transfers from cache and from NFN.
NFN computations and communication with
NFN-scala.
PIT for dedupblication and FIB for forwarding
this is true for NDN and CCNx

So I conclude from this, that none of the existing functions and features is broken for OSX and Unix.

RIOT should be evaluated by Cenk. We should wait for his OK

cgundogan · 2018-04-03T11:26:35Z

src/ccnl-fwd/src/ccnl-echo.c

@@ -71,8 +71,6 @@ ccnl_echo_request(struct ccnl_relay_s *relay, struct ccnl_face_s *inface,
    ucp = reply->data;
    len = reply->datalen;

-    while (!ccnl_switch_dehead(&ucp, &len, &enc)); // for iot2014 encoding


@blacksheeep I am not very familiar with this program. Is it safe to remove this line here (despite the comment saying it's just for iottlv, maybe the comment is wrong?)

Neither me, and we have a lack of documentation.

I think this functionality is only required to distinguish CISTLV and IOTTLV, so that it can be removed.

cgundogan

Here's a list of places in the code that still refer to IOT TLV:

mfrey · 2018-04-03T12:37:37Z

Hi,

thanks for the review! Comments below

https://github.com/mfrey/ccn-lite/blob/c28511cdec2bab17552cf165ad16a792c53ac3fb/src/ccnl-core/src/ccnl-frag.c#L49 (note about depcretation for IOT TLV?)

Added note on deprecation.

https://github.com/mfrey/ccn-lite/blob/c28511cdec2bab17552cf165ad16a792c53ac3fb/doc/internal/README-multiprotocol.md

This is kind of tricky, since

The following format numbers have been assigned so far:

  0  CCNL_ENC_CCNB
  1  CCNL_ENC_NDN2013
  2  CCNL_ENC_CCNX2014
  3  CCNL_ENC_IOT2014
  4  CCNL_ENC_LOCALRPC

Do the numbers refer to an enum or are they explicitly assigned? Okay this doesn't make any sense:

enum {
#ifdef USE_SUITE_CCNB
  CCNL_SUITE_CCNB = 1,
#endif
#ifdef USE_SUITE_CCNTLV
  CCNL_SUITE_CCNTLV = 2,
#endif
#ifdef USE_SUITE_CISTLV
  CCNL_SUITE_CISTLV = 3,
#endif
#ifdef USE_SUITE_IOTTLV
  CCNL_SUITE_IOTTLV = 4,
#endif
#ifdef USE_SUITE_LOCALRPC
  CCNL_SUITE_LOCALRPC = 5,
#endif
#ifdef USE_SUITE_NDNTLV
  CCNL_SUITE_NDNTLV = 6,
#endif
  CCNL_SUITE_LAST = 7
};

#define CCNL_SUITE_DEFAULT (CCNL_SUITE_LAST - 1)

// ----------------------------------------------------------------------
// our own packet format extension for switching encodings:
// 0x80 followed by:
// (add new encodings at the end)

enum {
  CCNL_ENC_CCNB,
  CCNL_ENC_NDN2013,
  CCNL_ENC_CCNX2014,
  CCNL_ENC_IOT2014,
  CCNL_ENC_LOCALRPC,
  CCNL_ENC_CISCO2015
};

I assume that some point the CCNL_SUITE* matched the values of the enum, but not anymore. CCNL_SUITE_NDNTLV != CCNL_ENC_NDN2013 (if it comes down to the stored value).

So, what do we do? I remove the value from README-multiprotocol.md and set the LocalRPC value to 4? Or I remove the table at all since it does not reflect all values or I extend it with the values? @cgundogan @blacksheeep any thoughts?

https://github.com/mfrey/ccn-lite/blame/c28511cdec2bab17552cf165ad16a792c53ac3fb/README.md#L175
https://github.com/mfrey/ccn-lite/blob/c28511cdec2bab17552cf165ad16a792c53ac3fb/src/CMakeLists.txt#L61
https://github.com/mfrey/ccn-lite/blob/c28511cdec2bab17552cf165ad16a792c53ac3fb/src/build-test.mk#L81
https://github.com/mfrey/ccn-lite/blob/c28511cdec2bab17552cf165ad16a792c53ac3fb/src/build-test.mk#L96
https://github.com/mfrey/ccn-lite/blob/c28511cdec2bab17552cf165ad16a792c53ac3fb/src/ccn-lite-simu.c#L42
https://github.com/mfrey/ccn-lite/blob/c28511cdec2bab17552cf165ad16a792c53ac3fb/src/ccnl-android/native/CMakeLists.txt#L44
https://github.com/mfrey/ccn-lite/blob/c28511cdec2bab17552cf165ad16a792c53ac3fb/src/ccnl-android/native/src/ccn-lite-android.c#L685

Removed.

https://github.com/mfrey/ccn-lite/blob/c28511cdec2bab17552cf165ad16a792c53ac3fb/src/ccnl-android/native/src/ccn-lite-jni.c#L153 (seems to be a bug here)

I would agree.

https://github.com/mfrey/ccn-lite/blob/c28511cdec2bab17552cf165ad16a792c53ac3fb/src/ccnl-core/include/ccnl-defs.h#L98
https://github.com/mfrey/ccn-lite/blob/c28511cdec2bab17552cf165ad16a792c53ac3fb/src/ccnl-core/include/ccnl-defs.h#L121
https://github.com/mfrey/ccn-lite/blob/c28511cdec2bab17552cf165ad16a792c53ac3fb/src/ccnl-core/src/ccnl-interest.c#L90
https://github.com/mfrey/ccn-lite/blob/c28511cdec2bab17552cf165ad16a792c53ac3fb/src/ccnl-core/src/ccnl-pkt.c#L61
https://github.com/mfrey/ccn-lite/blob/c28511cdec2bab17552cf165ad16a792c53ac3fb/src/ccnl-core/src/ccnl-pkt.c#L112
https://github.com/mfrey/ccn-lite/blob/c28511cdec2bab17552cf165ad16a792c53ac3fb/src/ccnl-core/src/ccnl-relay.c#L652
https://github.com/mfrey/ccn-lite/blob/c28511cdec2bab17552cf165ad16a792c53ac3fb/src/ccnl-lnxkernel/ccn-lite-lnxkernel.c#L42
https://github.com/mfrey/ccn-lite/blob/c28511cdec2bab17552cf165ad16a792c53ac3fb/src/doc/Doxyfile.in#L2064

Removed.

cgundogan · 2018-04-04T06:15:06Z

The following format numbers have been assigned so far:

0 CCNL_ENC_CCNB
1 CCNL_ENC_NDN2013
2 CCNL_ENC_CCNX2014
3 CCNL_ENC_IOT2014
4 CCNL_ENC_LOCALRPC

These (still) match to

enum {
  CCNL_ENC_CCNB,
  CCNL_ENC_NDN2013,
  CCNL_ENC_CCNX2014,
  CCNL_ENC_IOT2014,
  CCNL_ENC_LOCALRPC,
  CCNL_ENC_CISCO2015
};

In theory it should be fine just to remove CCNL_ENC_IOT2014 and from the README. I assume that nobody used the actual number, but rather the enum constant to refer to the appropriate value.

What we can do is:

assign numbers to the CCNL_ENC_ enum explicitly

enum {
  CCNL_ENC_CCNB = 0,
  CCNL_ENC_NDN2013 = 1,
  CCNL_ENC_CCNX2014 = 2,
  CCNL_ENC_LOCALRPC = 3,
  CCNL_ENC_CISCO2015 = 4
};

refer to the enum definition in the code. This would be so much simpler if we had a) doxygen for that enum and b) a compiled doc somewhere reachable from the webz. Just refering to the line number like I proposed before will break eventually if the file is touched. But we can go with that solution for now and adapt the README with a permanent link to the doxygen of that enum later.

blacksheeep · 2018-04-04T08:52:42Z

@cgundogan
We removed the numbers to be able to add new packet formats without confusion.
There shouldn't be trouble with that.
But yeah, doc would be good

mfrey · 2018-04-04T09:45:59Z

I've added doxygen documentation to the enum and also added a typedef. I removed the enum members from the readme (unfortunately, I can't point to an online documentation) and made a remark where to find the enum.

@cgundogan care to check the PR again?

cgundogan · 2018-04-04T12:35:50Z

doc/internal/README-multiprotocol.md

@@ -15,7 +15,7 @@ potentially confronted with packets encoded in different schemas like
 ccnb, CCNx2014 or NDN2013. In the past it was possible to discriminate
 among these packets because luckily enough the "type values" were
 sparsely populated and the programmers had chosen code points that do
-not overlap. But with the compact IOT2014 format, there is no sparsity
+not overlap. But with no packet formats, there might be no sparsity


But with no packet formats, there might be no sparsity

Hmm, this sounds weird now ..

Maybe just remove this block:

In the past it was possible to discriminate among these packets because luckily enough the "type values" were sparsely populated and the programmers had chosen code points that do not overlap. But with no packet formats, there might be no sparsity anymore.

?

That's right. It was supposed to be "new" and not "no". Would that work for you?

cgundogan

looks alright now, please squash

cgundogan

looks great. ACK

mfrey · 2018-04-05T09:55:45Z

okay, it builds (again?). could someone please approve?

cgundogan

ACK again

mfrey requested review from blacksheeep and cgundogan March 28, 2018 09:53

mfrey changed the title ~~removes iot tlv from ccn-lite~~ remove iot tlv from ccn-lite Mar 28, 2018

mfrey added the regression feature regression label Mar 28, 2018

blacksheeep reviewed Mar 28, 2018

View reviewed changes

cgundogan reviewed Apr 3, 2018

View reviewed changes

cgundogan requested changes Apr 3, 2018

View reviewed changes

mfrey force-pushed the rm-iottlv branch from c28511c to 537c78f Compare April 3, 2018 12:37

mfrey mentioned this pull request Apr 4, 2018

provide doxygen based documentation online #223

Open

cgundogan reviewed Apr 4, 2018

View reviewed changes

mfrey added the requires squashing commits need to be squashed before merge label Apr 4, 2018

cgundogan previously approved these changes Apr 4, 2018

View reviewed changes

mfrey dismissed cgundogan’s stale review via 1e6262b April 4, 2018 14:42

mfrey force-pushed the rm-iottlv branch 2 times, most recently from 1e6262b to 6701675 Compare April 4, 2018 14:48

mfrey removed the requires squashing commits need to be squashed before merge label Apr 4, 2018

cgundogan previously approved these changes Apr 4, 2018

View reviewed changes

blacksheeep self-requested a review April 4, 2018 15:08

blacksheeep previously approved these changes Apr 4, 2018

View reviewed changes

mfrey dismissed stale reviews from blacksheeep and cgundogan via 14f88d9 April 4, 2018 15:12

mfrey force-pushed the rm-iottlv branch from 6701675 to 14f88d9 Compare April 4, 2018 15:12

blacksheeep previously approved these changes Apr 4, 2018

View reviewed changes

mfrey dismissed blacksheeep’s stale review via 2a2a40b April 5, 2018 09:25

mfrey force-pushed the rm-iottlv branch from 14f88d9 to 2a2a40b Compare April 5, 2018 09:25

removed iottlv packet format

8e79aec

mfrey force-pushed the rm-iottlv branch from 2a2a40b to 8e79aec Compare April 5, 2018 09:54

cgundogan approved these changes Apr 5, 2018

View reviewed changes

blacksheeep merged commit 6c3a8cd into cn-uofbasel:master Apr 5, 2018

mfrey mentioned this pull request Apr 5, 2018

remove cis tlv from ccn-lite #228

Merged

mfrey deleted the rm-iottlv branch April 24, 2018 07:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove iot tlv from ccn-lite #222

remove iot tlv from ccn-lite #222

mfrey commented Mar 28, 2018 •

edited

Loading

blacksheeep left a comment

cgundogan Apr 3, 2018

blacksheeep Apr 3, 2018

cgundogan left a comment

mfrey commented Apr 3, 2018 •

edited

Loading

cgundogan commented Apr 4, 2018

blacksheeep commented Apr 4, 2018

mfrey commented Apr 4, 2018

cgundogan Apr 4, 2018

mfrey Apr 4, 2018 •

edited

Loading

cgundogan left a comment

cgundogan left a comment

mfrey commented Apr 5, 2018

cgundogan left a comment

remove iot tlv from ccn-lite #222

remove iot tlv from ccn-lite #222

Conversation

mfrey commented Mar 28, 2018 • edited Loading

blacksheeep left a comment

Choose a reason for hiding this comment

cgundogan Apr 3, 2018

Choose a reason for hiding this comment

blacksheeep Apr 3, 2018

Choose a reason for hiding this comment

cgundogan left a comment

Choose a reason for hiding this comment

mfrey commented Apr 3, 2018 • edited Loading

cgundogan commented Apr 4, 2018

blacksheeep commented Apr 4, 2018

mfrey commented Apr 4, 2018

cgundogan Apr 4, 2018

Choose a reason for hiding this comment

mfrey Apr 4, 2018 • edited Loading

Choose a reason for hiding this comment

cgundogan left a comment

Choose a reason for hiding this comment

cgundogan left a comment

Choose a reason for hiding this comment

mfrey commented Apr 5, 2018

cgundogan left a comment

Choose a reason for hiding this comment

mfrey commented Mar 28, 2018 •

edited

Loading

mfrey commented Apr 3, 2018 •

edited

Loading

mfrey Apr 4, 2018 •

edited

Loading