JulianSchmid / etherparse

A rust library for parsing ethernet & ethernet using protocols.
Apache License 2.0
286 stars 54 forks source link

Implement PacketBuilderStep<IpHeader>.write #27

Closed karpawich closed 2 years ago

karpawich commented 2 years ago

Resolves #26

@JulianSchmid : what is missing from this PR? I can help write tests, docs, etc. but I want to ask you for your guidance first.

codecov[bot] commented 2 years ago

Codecov Report

Merging #27 (579ebeb) into master (5016f3a) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           master      #27    +/-   ##
========================================
  Coverage   99.62%   99.62%            
========================================
  Files          51       51            
  Lines       21337    21532   +195     
========================================
+ Hits        21256    21452   +196     
+ Misses         81       80     -1     
Impacted Files Coverage Δ
src/internet/ipv4.rs 100.00% <100.00%> (ø)
src/packet_builder.rs 100.00% <100.00%> (+0.21%) :arrow_up:
tests/internet/ipv4.rs 100.00% <100.00%> (ø)
tests/packet_builder/mod.rs 100.00% <100.00%> (ø)
tests/transport/icmpv4.rs 99.19% <100.00%> (-0.01%) :arrow_down:
tests/transport/icmpv6.rs 99.70% <100.00%> (-0.01%) :arrow_down:
tests/transport/tcp.rs 99.78% <100.00%> (ø)
src/transport/tcp.rs 99.88% <0.00%> (-0.01%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 5016f3a...579ebeb. Read the comment docs.

JulianSchmid commented 2 years ago

Hi @karpawich ,

I don't know when I get around to reviewing your PR in depth. But add some tests here https://github.com/JulianSchmid/etherparse/blob/master/tests/packet_builder/mod.rs for the new code paths you added.

You can use the coverage report to find holes and use the script https://github.com/JulianSchmid/etherparse/tree/master/scripts to calculate a coverage report locally.

Greets & 1000 Thanks Julian

JulianSchmid commented 2 years ago

I forgot to mention: Thanks for the PR :)

karpawich commented 2 years ago

I have written the tests. I still need to update the documentation.

N.B: The protocol argument of the Ipv4Header::new function needs to be of type u8 instead of IpNumber to support the use of unassigned IP numbers. If it did not introduce breaking changes, I would also suggest renaming IpNumber to AssignedIpNumber.

JulianSchmid commented 2 years ago

Hi @karpawich , I will try to have a look at your PR this weekend.