ng_net: introduce checksum calculation #2553

miri64 · 2015-03-07T16:05:50Z

The idea is that IPv6 (and later IPv4), after determining the source address, can call this new function and without actual knowledge of the calculation method for the checksum.

haukepetersen · 2015-03-10T14:43:23Z

sys/include/net/ng_netif/hdr.h

+ * @return  The generic network layer header on success.
+ * @return  NULL on error.
+ */
+static inline ng_pktsnip_t *ng_netif_hdr_build(ng_pktsnip_t *payload,


don't like the payload parameter here -> as we can use this function then only for receiving (in the current form). I would rather allocate the interface header stand-alone and link it to the rest of the packtsnips manually...

Did you read the doc?

* @brief Builds a generic network interface header for sending and

* adds it to the packet buffer.

The reason why it looks like for receiving is because I had it reversed :D

But you are probably right, nevertheless…

haukepetersen · 2015-03-10T14:46:20Z

I would vote for separating this PR in two: one for header creation and one for cksum calculation.

-> the checksum part I think looks good, with the header creation stuff I still have some problems, but I can't really say right now what it is...

miri64 · 2015-03-10T22:32:44Z

Moved header creation to #2575 and rebased to current master.

cgundogan · 2015-03-12T08:37:23Z

sys/net/crosslayer/ng_netreg/ng_netreg.c

+                             ng_pktsnip_t *pkt)
+{
+    switch (csum_hdr_type) {
+#ifdef MODULE_NG_ICMPV6


A possible alternative to this ifdefapproach may be to put all csum functions (used below) in a common place with empty bodies and the __(weak)__ attribute to be able to overwrite them by their appropriate modules. This would remove the ifdef-ugliness and makes this part of code more readable. I am not sure how smart the compiler is, however, to optimize away the case statements for function calls, which have empty bodies.

I guess you cannot define empty-body functions, when these functions need to return something else then void, can you? :/ This would nullify my idea, so I think...

EDIT:
... at least in terms of optimization, not in terms of readability.

The bigger problem is not that the functions are not defined, but the different values for ng_nettype_t for different modules used, due to size optimization for the ng_netreg look-up table. If the UDP module does not exist at compile time, NG_NETTYPE_UDP is not defined for example.

miri64 · 2015-03-17T09:54:52Z

@haukepetersen ping?

miri64 · 2015-03-19T14:42:25Z

Rebased to current master

haukepetersen · 2015-03-23T14:42:19Z

sys/include/net/ng_netreg.h

+ *
+ * @param[in] pseudo_hdr_type   The type of the pseudo header if required.
+ *                              May be NG_NETTYPE_UNDEF if no pseudo header is
+ *                              required for the checksum calculation.


You could be more consistent with the use of periods at the end of the comments -> I would omit them...

Done.
Am 23.03.2015 15:42 schrieb "Hauke Petersen" [email protected]:

In sys/include/net/ng_netreg.h
#2553 (comment):

@@ -126,6 +126,30 @@ 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 Calculates the checksum for a packet of type @p checksum_hdr_type.

* @param[in] pseudo_hdr_type The type of the pseudo header if required.

* May be NG_NETTYPE_UNDEF if no pseudo header is

* required for the checksum calculation.

You could be more consistent with the use of periods at the end of the
comments -> I would omit them...

—
Reply to this email directly or view it on GitHub
https://github.com/RIOT-OS/RIOT/pull/2553/files#r26942061.

Lotterleben · 2015-03-24T11:18:50Z

looks good to me...

miri64 · 2015-03-24T17:29:43Z

@Lotterleben was this an ACK?

Lotterleben · 2015-03-24T17:43:58Z

@authmillenon Yup. But. I think it needs one more squash :)

miri64 · 2015-03-24T17:47:49Z

Done

Lotterleben · 2015-03-24T17:48:06Z

👍

miri64 · 2015-03-24T17:55:51Z

Now we only have to wait a few days, until Travis gets its thing together -.-

Lotterleben · 2015-03-24T18:07:41Z

🎈 go Travis!

haukepetersen · 2015-03-25T23:31:47Z

sys/net/crosslayer/ng_netreg/ng_netreg.c

@@ -19,6 +19,7 @@
 #include "utlist.h"
 #include "net/ng_netreg.h"
 #include "net/ng_nettype.h"
+#include "net/ng_ipv6.h"


my compiler just told me that you also have to include net/ng_pkt.h

haukepetersen · 2015-03-25T23:32:16Z

please fix the issue above before merging.

haukepetersen · 2015-03-25T23:35:23Z

same for ng_netreg.h, so including net/ng_pkt.h in ng_netreg.h should be sufficient.

haukepetersen · 2015-03-25T23:59:55Z

Sorry for bringing this up so late: I just started to implemented the checksum function for UDP, and I might just had a slightly nicer idea for the checksum calculation API:

int ng_netreg_calculate_csum(ng_pktsnip_t *hdr, ng_pktsnip_t *pseudo_hdr)
{
    switch (hdr->type) {
   [...]
#ifdef MODULE_NG_UDP

        case NG_NETTYPE_UDP:
            return ng_udp_calculate_csum(hdr, pseudo_hdr);
#endif
   [...]
}

// and 
ng_udp_calculate_csum(pktsnip_t *hdr, pktsnip_t *pseudo_hdr)
{
    switch (pseudo_hdr) {
        case NG_NETTYPE_IPV6:
// you get the idea...

Make the interface a little easier to use and nicer to look at. What do you think?

haukepetersen · 2015-03-26T00:44:30Z

It's getting late, but if we have the assumption, that this function is only called when sending out data, it should even be enough to just give the pointer to network layer header (IP), as the transport header and the payload are chained after it:

int ng_netreg_calcualte_csum(ng_pktsnip_t *pseudo_hdr)
{
    switch (pseudo_hdr->next->type) {
...

miri64 · 2015-03-26T04:44:05Z

It's getting late, but if we have the assumption, that this function is only called when sending out data, it should even be enough to just give the pointer to network layer header (IP), as the transport header and the payload are chained after it:

You are forgetting Extension Headers once again, but with the two-header solution I might be able to live.

miri64 · 2015-03-26T05:17:37Z

@haukepetersen Have a look.

haukepetersen · 2015-03-26T10:54:38Z

sys/net/crosslayer/ng_netreg/ng_netreg.c

+    switch (hdr->type) {
+#ifdef MODULE_NG_ICMPV6
+        case NG_NETTYPE_ICMPV6:
+            return (pseudo_hdr) ? ng_icmpv6_calc_csum(hdr, pseudo_hdr) : -EINVAL;


keep it simple, just do the if (pseudo_hdr == NULL) { return -EINVAL; } at the beginning of the fuction. Or compile both solutions and show that this notation acutally saves memory (which I say it does the contrary) :-)

I'd rather be prepared for future protocol implementations where no pseudo header is required ;-).

I see. But still, I would go with the 'standard notation' for now (as it saves memory), and switch to this notation here whenever we add a protocol that does not need a pseudo header.

What do you mean with "standard notation"?

if (pseudo_hdr) { return ng_icmpv6_calc_csum(hdr, pseudo_hdr); } else { return -EINVAL;

instead of the ternary operator?

haukepetersen · 2015-03-26T10:55:10Z

Yes, always these extension-headers :-)

miri64 · 2015-03-28T16:56:03Z

Moved NULL pointer check for pseudo_hdr and squashed so we can get this thing merged already!

miri64 · 2015-03-29T12:55:39Z

Rebased to current master.

miri64 · 2015-03-30T00:41:18Z

@haukepetersen does the ACK uphold?

Lotterleben · 2015-04-01T12:56:12Z

@haukepetersen ping? :>

miri64 · 2015-04-09T19:55:48Z

I guess since he uses it this way in #2430 he is okay with it, so I think you can make an executive decision, @Lotterleben (if not we can always change this one later ;-))

Lotterleben · 2015-04-10T00:01:06Z

True.. I just didn't want to merge this over @haukepetersen s head, since he had the stronger opinions here. Let's give it a go. :)

ng_net: introduce checksum calculation

haukepetersen · 2015-04-13T12:21:12Z

sorry for being in-responsive... I think this is fine for now, let's re-visit this maybe at some later point and see if we can optimize it.

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 7, 2015

miri64 added this to the Network Stack Task Force milestone Mar 7, 2015

miri64 assigned haukepetersen Mar 7, 2015

miri64 force-pushed the net/feat/hdr-csum branch 2 times, most recently from 7e4aa6e to 9e63509 Compare March 7, 2015 16:10

miri64 mentioned this pull request Mar 7, 2015

ng_ipv6: initial import #2454

Merged

haukepetersen reviewed Mar 10, 2015
View reviewed changes

miri64 force-pushed the net/feat/hdr-csum branch from 87bb904 to 2d69105 Compare March 10, 2015 22:28

miri64 mentioned this pull request Mar 10, 2015

ng_net: header building facilities #2575

Merged

miri64 changed the title ~~ng_net: introduce checksum calculation and header building facilities~~ ng_net: introduce checksum calculation Mar 10, 2015

cgundogan reviewed Mar 12, 2015
View reviewed changes

miri64 force-pushed the net/feat/hdr-csum branch from 2d69105 to 67e7c49 Compare March 19, 2015 14:42

miri64 assigned Lotterleben and unassigned haukepetersen Mar 23, 2015

haukepetersen reviewed Mar 23, 2015
View reviewed changes

miri64 force-pushed the net/feat/hdr-csum branch from 224b254 to 4c33b1a Compare March 24, 2015 17:47

haukepetersen reviewed Mar 25, 2015
View reviewed changes

miri64 mentioned this pull request Mar 26, 2015

net: added ng_udp implementation #2430

Merged

haukepetersen reviewed Mar 26, 2015
View reviewed changes

miri64 force-pushed the net/feat/hdr-csum branch from 21ab32e to c12c49f Compare March 28, 2015 16:55

netreg: add multiplexer for checksum calculation

4571fe2

miri64 force-pushed the net/feat/hdr-csum branch from c12c49f to 4571fe2 Compare March 29, 2015 12:55

Lotterleben force-pushed the master branch from 1d9f189 to d4e068b Compare March 31, 2015 09:52

OlegHahm force-pushed the master branch 3 times, most recently from 9f184dd to 45554bf Compare March 31, 2015 13:01

Lotterleben added a commit that referenced this pull request Apr 10, 2015

Merge pull request #2553 from authmillenon/net/feat/hdr-csum

e130b69

ng_net: introduce checksum calculation

Lotterleben merged commit e130b69 into RIOT-OS:master Apr 10, 2015

miri64 deleted the net/feat/hdr-csum branch April 10, 2015 05:40

ng_net: introduce checksum calculation #2553

ng_net: introduce checksum calculation #2553

Conversation

miri64 commented Mar 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haukepetersen commented Mar 10, 2015

miri64 commented Mar 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miri64 commented Mar 17, 2015

miri64 commented Mar 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lotterleben commented Mar 24, 2015

miri64 commented Mar 24, 2015

Lotterleben commented Mar 24, 2015

miri64 commented Mar 24, 2015

Lotterleben commented Mar 24, 2015

miri64 commented Mar 24, 2015

Lotterleben commented Mar 24, 2015

Choose a reason for hiding this comment

haukepetersen commented Mar 25, 2015

haukepetersen commented Mar 25, 2015

haukepetersen commented Mar 25, 2015

haukepetersen commented Mar 26, 2015

miri64 commented Mar 26, 2015

miri64 commented Mar 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haukepetersen commented Mar 26, 2015

miri64 commented Mar 28, 2015

miri64 commented Mar 29, 2015

miri64 commented Mar 30, 2015

Lotterleben commented Apr 1, 2015

miri64 commented Apr 9, 2015

Lotterleben commented Apr 10, 2015

haukepetersen commented Apr 13, 2015