Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom

net_help: Cleanup #2019

Merged
merged 3 commits into from
Nov 18, 2014
Merged

net_help: Cleanup #2019

merged 3 commits into from
Nov 18, 2014

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Nov 17, 2014

In addition to #1984 this cleans up the net_help module.

  • removes printArrayRange(). Alternative: the od module
  • removes IPV6_CMP_ADDR(). Alternative: ipv6_addr_is_equal()/ipv6_addr_equal() (as of WIP IPv6 refactoring #2003). Other comparisons between IPv6 addresses would be odd anyway
  • renames csum() according to Coding Conventions to net_help_csum()

@miri64
net_help: remove printArrayRange()
53eaa4f 
The od module does the same, much less specialized, much more
sophisticated.
@miri64
net_help: remove IPV6_CMP_ADDR macro
4d82c2c 
Used nowhere; alternative: ipv6_addr_is_equal(), since other use-cases
(is an IPv6 address smaller than the other) are not applicable anyway.
@miri64 miri64 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer labels Nov 17, 2014
@miri64 miri64 added this to the Release NEXT MAJOR milestone Nov 17, 2014
@miri64 miri64 added the Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. label Nov 17, 2014
@miri64
Copy link
Member Author

miri64 commented Nov 17, 2014

After testing this function I have to admit I really don't understand what this function does :/

OlegHahm
OlegHahm reviewed Nov 17, 2014
View reviewed changes
sys/net/crosslayer/net_help/net_help.c
@@ -44,13 +32,13 @@ uint16_t csum(uint16_t sum, uint8_t *buf, uint16_t len)
buf += 2;
sum += carry;
sum += t;
carry = (t > sum);
carry = (t > sum) ? 1 : 0;
Copy link
Member

Choose a reason for hiding this comment

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

I find the original code more readable.

@OlegHahm
Copy link
Member

csum() does - as its name suggests - calculate the next header checksum (for UDP, TCP, and ICMP) as described in the RFC. Therefore, I would vote for renaming to net_help_csum().

@miri64
Copy link
Member Author

miri64 commented Nov 17, 2014

But at least in ICMP (where it is used to calculate the checksum, and where the checksum is [in most cases ;-)] correct according to wireshark) it's supposed to be

16-bit one's complement of the one's complement sum of the entire ICMPv6 message […] [1]

But I neither see the one's complement, nor is it the sum as I would expect a sum would look like.

@miri64
Copy link
Member Author

miri64 commented Nov 17, 2014

Reverted the negatively criticized changes.

@miri64
Copy link
Member Author

miri64 commented Nov 17, 2014

Remembered my telematics classes and the Internet Checksum. Updated the documentation accordingly.

@OlegHahm OlegHahm added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Nov 18, 2014
@OlegHahm
Copy link
Member

ACk. Squash and merge at will.

@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 Nov 18, 2014
miri64 added a commit that referenced this pull request Nov 18, 2014
@miri64
Merge pull request #2019 from authmillenon/cleanup-net_help
4d674ef 
net_help: Cleanup
@miri64 miri64 merged commit 4d674ef into RIOT-OS:master Nov 18, 2014
@miri64 miri64 deleted the cleanup-net_help branch November 18, 2014 15:41
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers
No reviews
Assignees

@OlegHahm OlegHahm

Labels
Area: network Area: Networking Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
Release 2014.12
Development

Successfully merging this pull request may close these issues.
2 participants
@miri64 @OlegHahm