Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ng_net: header building facilities #2575

Merged
merged 2 commits into from
Mar 29, 2015
Merged

ng_net: header building facilities #2575

merged 2 commits into from
Mar 29, 2015

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Mar 10, 2015

This was taken out of #2553

The idea is that an upper-layer can build a header for the layer it wants to send data over without knowledge about the actual structure of the header. I also added a convinience function of similar format to ng_netif_hdr.

Depends on #2706 (merged)

@miri64 miri64 added Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. NSTF labels Mar 10, 2015
@miri64 miri64 added this to the Network Stack Task Force milestone Mar 10, 2015
This was referenced Mar 10, 2015
Merged
Merged
OlegHahm
OlegHahm reviewed Mar 13, 2015
View reviewed changes
sys/include/net/ng_netif/hdr.h
@@ -134,6 +136,46 @@ static inline void ng_netif_hdr_set_dst_addr(ng_netif_hdr_t *hdr, uint8_t *addr,
memcpy(((uint8_t *)(hdr + 1)) + hdr->src_l2addr_len, addr, addr_len);
}

/**
* @brief Builds a generic network interface header for sending and
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/network interface/network layer/ ? But what does "generic" mean in this context?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No this is correct. This module (ng_netif_hdr) defines the "generic network interface header". This name was introduced after @OlegHahm was not happy with the name "generic link layer header" ;-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused. I thought this was meant to implement (in a IMO weird way) IPv6 upper layer checksum calculation. Why do one need to build a generic network interface header?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No that's in #2553. This just offers netif_hdr_init() and pktbuf_add() in one go.

@OlegHahm
Copy link
Member

Sorry, I guess I asked this a 100 times before, but I'm confused again why an upper layer wants to build a lower layer's header?

@miri64
Copy link
Member Author

miri64 commented Mar 13, 2015

Use-case: IPv6 wants to send packet.

  • IPv6 checks next hop
  • IPv6 searches link-layer address for next hop
  • wants to tell link layer were to send it (and maybe additional information): uses link-layer header.

@miri64
Copy link
Member Author

miri64 commented Mar 13, 2015

Why not just the address? Because in the rest of the model we use the header for exactly the same reason as I sketched out previously: The upper-layer might want to set additional information (hop-limit, traffic class, etc. you name it).

@OlegHahm
Copy link
Member

The upper-layer might want to set additional information (hop-limit, traffic class, etc. you name it).

But this would require an additional function, right? Why can I do this not just with a set_opt command?

@miri64
Copy link
Member Author

miri64 commented Mar 13, 2015

Because this requires additional context switches. And no it would not need additional functions. Once you've build the header, you know the type of the header anyways, so if you need to, you can just change the fields after the creation. This functions are primarily for service functions for layers that DON'T need to know the build-up of a lower-layer header.

@miri64
Copy link
Member Author

miri64 commented Mar 19, 2015

Rebased to current master

@miri64 miri64 mentioned this pull request Mar 22, 2015
Merged
@miri64 miri64 assigned Lotterleben and unassigned haukepetersen Mar 23, 2015
Lotterleben
Lotterleben reviewed Mar 24, 2015
View reviewed changes
sys/include/net/ng_netreg.h
@@ -126,6 +127,26 @@ int ng_netreg_num(ng_nettype_t type, uint32_t demux_ctx);
*/
ng_netreg_entry_t *ng_netreg_getnext(ng_netreg_entry_t *entry);

/**
* @brief Builds a header for sending for a protocol in the registry and
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: I find this sentence a bit hard to parse. Maybe the “for sending” could be left out?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, since a receiving packet would store the payload differently.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. Then maybe we could split it up into two sentences or something?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Builds a header for sending and adds it to the packet buffer" would also work, I think. That the protocol is defined through type is apparent through the type parameter I think.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@miri64
Copy link
Member Author

miri64 commented Mar 24, 2015

Simplified doc a little.

@miri64
Copy link
Member Author

miri64 commented Mar 25, 2015

Needed to add #2706 as a depenency since Travis is taking forever -.-

@miri64 miri64 added CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable State: waiting for other PR State: The PR requires another PR to be merged first labels Mar 25, 2015
@Lotterleben
Copy link
Member

Awww come on Travis!

@Kijewski
Copy link
Contributor

The last successful Travis run is two and a half days old. I think we've killed that poor fellow.

@saurabh984
Copy link

I'll sponsor a dedicated server if anyone fancies setting up Janky (or Jenkins) on it! Travis is a nighmare

@miri64
Copy link
Member Author

miri64 commented Mar 25, 2015

@saurabh984 we're (or rather @phiros is) working on a solution. We had Jenkins in the past. This was also no solution, since someone(TM) needs to manage that machine and nobody got time for that.

@miri64
Copy link
Member Author

miri64 commented Mar 25, 2015

It's probably gonna be build bot.

@miri64
Copy link
Member Author

miri64 commented Mar 25, 2015

But thanks for the offer

@miri64
Copy link
Member Author

miri64 commented Mar 25, 2015

Fixed thingie

@miri64
Copy link
Member Author

miri64 commented Mar 26, 2015

Moved author tag.

This was referenced Mar 26, 2015
Closed
Merged
@miri64 miri64 removed the State: waiting for other PR State: The PR requires another PR to be merged first label Mar 28, 2015
@miri64
Copy link
Member Author

miri64 commented Mar 28, 2015

Apart from the squash and PR dependency warning and an aborted (and now restarted) build in the msp430 group travis seems to be fine. Squash?

@Lotterleben
Copy link
Member

Yes please!

@miri64
Copy link
Member Author

miri64 commented Mar 28, 2015

Done

@Lotterleben
Copy link
Member

👍 Okay... Let's do this! ACK & Go.

@Lotterleben
Copy link
Member

Correction: ACK, wait for Travis & go. I'll keep this tab open.

@miri64
Copy link
Member Author

miri64 commented Mar 28, 2015

Ahhh sorry… I needed to change something for constistency: s/dest/dst/

@Lotterleben
Copy link
Member

:D good catch! Tab is still open, will merge as soon as Travis is happy.

@OlegHahm
Copy link
Member

I would like to put this topic on the list for the next NSTF meeting. I'm not nacking this, but am still not convinced of this approach.

@miri64
Copy link
Member Author

miri64 commented Mar 29, 2015

@OlegHahm can you point me to the problem with this approach?

@miri64 miri64 removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Mar 29, 2015
miri64 added a commit that referenced this pull request Mar 29, 2015
@miri64
Merge pull request #2575 from authmillenon/ng_net/feat/hdr-build
fa689fe 
ng_net: header building facilities
@miri64 miri64 merged commit fa689fe into RIOT-OS:master Mar 29, 2015
@miri64 miri64 deleted the ng_net/feat/hdr-build branch March 29, 2015 12:28
@OlegHahm
Copy link
Member

No, I just still don't like upper layers building lower layers' headers. But it's so far more like a gut feeling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Lotterleben Lotterleben

Labels
Area: network Area: Networking Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Milestone
Network Stack Task Force
Development

Successfully merging this pull request may close these issues.
6 participants
@miri64 @OlegHahm @Lotterleben @Kijewski @saurabh984 @haukepetersen