gnrc_pktbuf: add gnrc_pktbuf_reverse_snips() helper function by miri64 · Pull Request #10226 · RIOT-OS/RIOT

miri64 · 2018-10-23T11:23:14Z

Contribution description

This allows for

a) testing the packet reversal properly in unittests
b) use it in other places than gnrc_ipv6's receive function

Testing procedure

Run the newly provided unittests:

make -C tests/unittests tests-pktbuf test

If that succeeds, try to compile gnrc_networking. If you really want to be thorough, set up a multihop network with gnrc_networking and see if forwarding still works (it should, but I did not test that).

Issues/PRs references

jnohlgard

Some comments inline

jnohlgard · 2018-10-24T04:57:48Z

sys/include/net/gnrc/pktbuf.h

 gnrc_pktsnip_t *gnrc_pktbuf_replace_snip(gnrc_pktsnip_t *pkt, gnrc_pktsnip_t *old, gnrc_pktsnip_t *add);

+/**
+ * @brief   Reverses snip order of a packet securely.


What does securely mean here?

Write-protected. If ptr->users > 2 it is assuered that a copy is made before

Will clarify.

jnohlgard · 2018-10-24T04:59:51Z

sys/net/gnrc/pktbuf/gnrc_pktbuf.c

+
+    for (gnrc_pktsnip_t *ptr = pkt; ptr != NULL;
+         /* progress within loop, as next pointers get switched out */) {
+        gnrc_pktsnip_t *next = NULL;


This definition could be moved to L105

I always put definitions on top of blocks, so I don't loose track of the variables used within it, but it is true, that the = NULL assignment is unnecessary here.

jnohlgard · 2018-10-24T05:01:16Z

sys/net/gnrc/pktbuf/gnrc_pktbuf.c

+    gnrc_pktsnip_t *reversed = NULL;
+
+    for (gnrc_pktsnip_t *ptr = pkt; ptr != NULL;
+         /* progress within loop, as next pointers get switched out */) {


This is a weird line wrap

I know... it's also weird, that there is no progress statement. Do you have a better idea?

What do you think of this?

gnrc_pktsnip_t *ptr = pkt; while(ptr) {

Usually I think for loops are better to read, but I agree that in this instance while() might be better :-). Will adapt.

sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c

jnohlgard · 2018-10-24T07:01:19Z

sys/net/gnrc/pktbuf/gnrc_pktbuf.c

+        gnrc_pktsnip_t *next = NULL;
+
+        /* use pkt as temporary variable */
+        pkt = gnrc_pktbuf_start_write(ptr);


Why does this happen on every iteration inside the loop?

Because gnrc_pktbuf_start_write() only write-protects snip-wise. To keep write-protection for the whole packet we need to call it for every snip we change the next pointer for (so all of them ;-)).

Will add a comment.

miri64 · 2018-10-24T07:26:20Z

@gebart I addressed your comments.

miri64 · 2018-10-25T20:24:47Z

@gebart @Lotterleben any more comments or may I squash?

Lotterleben · 2018-10-25T20:34:00Z

@miri64 No more documentation nitpicks from me :)

jnohlgard · 2018-10-25T21:05:37Z

The docs for gnrc_pktbuf_start_write could use a rephrasing. Currently it says:

gnrc_pktsnip_t* gnrc_pktbuf_start_write(gnrc_pktsnip_t *pkt)
Must be called once before there is a write operation in a thread.

This function duplicates a packet in the packet buffer if gnrc_pktsnip_t::users of pkt > 1.

Note
Do not call this function in a thread twice on the same packet.

That is a separate PR though.

jnohlgard

Looks fine now. OK to squash

miri64 · 2018-10-25T21:10:42Z

The docs for gnrc_pktbuf_start_write could use a rephrasing

You did not give any details on how it needs rephrasing. Do you mind to open a PR how you feel the rephrasing is required?

This allows for a) testing the packet reversal properly in unittests b) use it in other places than `gnrc_ipv6`'s receive function

miri64 · 2018-10-25T21:14:28Z

@miri64 No more documentation nitpicks from me :)

@gebart did you intend for @Lotterleben to set the 5-documentation label? (@Lotterleben since I believe you did not review in a long time, here's the reference to what I am talking about).

miri64 · 2018-10-25T21:16:48Z

(squashed btw)

miri64 · 2018-10-25T21:38:35Z

Unittests passed on native.

jnohlgard · 2018-10-25T21:55:08Z

The docs for gnrc_pktbuf_start_write could use a rephrasing

You did not give any details on how it needs rephrasing. Do you mind to open a PR how you feel the rephrasing is required?

Opened #10261
My main confusion is the use of the word packet to refer to a gnrc snip.

miri64 requested review from bergzand and cgundogan October 23, 2018 11:23

miri64 force-pushed the gnrc_pktbuf/enh/reverse-snips branch 2 times, most recently from cef9e11 to 58b8d08 Compare October 23, 2018 11:34

miri64 added a commit to miri64/RIOT that referenced this pull request Oct 23, 2018

Merge branch 'gnrc_pktbuf/enh/reverse-snips' (RIOT-OS#10226) into HEAD

74fc0bf

miri64 mentioned this pull request Oct 23, 2018

gnrc_ipv6_ext: use send for forwarding with RH #10229

Merged

miri64 added a commit to miri64/RIOT that referenced this pull request Oct 23, 2018

Merge branch 'gnrc_pktbuf/enh/reverse-snips' (RIOT-OS#10226) into HEAD

a09bef9

miri64 added a commit to miri64/RIOT that referenced this pull request Oct 23, 2018

Merge branch 'gnrc_pktbuf/enh/reverse-snips' (RIOT-OS#10226) into HEAD

6b9795e

jnohlgard reviewed Oct 24, 2018

View reviewed changes

miri64 removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 24, 2018

miri64 added a commit to miri64/RIOT that referenced this pull request Oct 25, 2018

Merge branch 'gnrc_pktbuf/enh/reverse-snips' (RIOT-OS#10226) into HEAD

b6014ed

miri64 mentioned this pull request Oct 25, 2018

gnrc_ipv6: move ipv6_ext iteration out of ext_demux() #10234

Merged

jnohlgard approved these changes Oct 25, 2018

View reviewed changes

miri64 added 3 commits October 25, 2018 23:11

gnrc_pktbuf: add gnrc_pktbuf_reverse_snips() helper function

ac54a2d

This allows for a) testing the packet reversal properly in unittests b) use it in other places than `gnrc_ipv6`'s receive function

gnrc_ipv6: use gnrc_pktbuf_reverse_snips()

a5c9f95

unittests: add tests for gnrc_pktbuf_reverse_snips()

d6ea335

miri64 force-pushed the gnrc_pktbuf/enh/reverse-snips branch from 7c748b4 to d6ea335 Compare October 25, 2018 21:11

miri64 added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Oct 25, 2018

miri64 merged commit f597c6a into RIOT-OS:master Oct 25, 2018

miri64 deleted the gnrc_pktbuf/enh/reverse-snips branch October 25, 2018 21:51

jnohlgard mentioned this pull request Oct 25, 2018

gnrc_pktbuf_start_write API docs unclear #10261

Closed

Conversation

miri64 commented Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

jnohlgard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Oct 24, 2018

Uh oh!

miri64 commented Oct 25, 2018

Uh oh!

Lotterleben commented Oct 25, 2018

Uh oh!

jnohlgard commented Oct 25, 2018

Uh oh!

jnohlgard left a comment

Choose a reason for hiding this comment

Uh oh!

miri64 commented Oct 25, 2018

Uh oh!

miri64 commented Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 commented Oct 25, 2018

Uh oh!

miri64 commented Oct 25, 2018

Uh oh!

jnohlgard commented Oct 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

miri64 commented Oct 23, 2018 •

edited

Loading

miri64 commented Oct 25, 2018 •

edited

Loading