Add ARP support #103

lieka1 · 2024-07-02T07:04:14Z

Hi there! i dont' know if you are accepting any pr for arp support

added:
SlicedPacket::from_ethernet is now supporting arp EtherType
PacketBuilderStep is added to build arp header

but LaxSlicedPacket::from_ethernet still cannot parse arp package, i am not sure if is should add it since arp header is quite short

JulianSchmid · 2024-07-03T19:49:04Z

Hi @lieka1 ,

I will have a look at your PR this weekend. Thanks in advance.

Greets
Julian

codecov · 2024-07-06T05:23:22Z

Codecov Report

Attention: Patch coverage is 95.36560% with 90 lines in your changes missing coverage. Please review.

Project coverage is 99.25%. Comparing base (93c7f0b) to head (80f5ddf).
Report is 20 commits behind head on master.

Files with missing lines	Patch %	Lines
etherparse/src/lax_sliced_packet.rs	0.00%	13 Missing ⚠️
etherparse/src/lax_packet_headers.rs	85.52%	11 Missing ⚠️
etherparse/src/lax_sliced_packet_cursor.rs	8.33%	11 Missing ⚠️
etherparse/src/sliced_packet.rs	92.61%	11 Missing ⚠️
etherparse/src/packet_headers.rs	16.66%	10 Missing ⚠️
etherparse/src/packet_builder.rs	97.05%	9 Missing ⚠️
etherparse/src/compositions_tests.rs	89.61%	8 Missing ⚠️
etherparse/src/net/net_headers.rs	80.00%	4 Missing ⚠️
etherparse/src/net/net_slice.rs	0.00%	4 Missing ⚠️
etherparse/src/err/layer.rs	0.00%	2 Missing ⚠️
... and 6 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #103      +/-   ##
==========================================
- Coverage   99.31%   99.25%   -0.06%     
==========================================
  Files         168      177       +9     
  Lines       29895    31788    +1893     
==========================================
+ Hits        29689    31552    +1863     
- Misses        206      236      +30

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

JulianSchmid

Hi @lieka1,

Firstly thanks for the PR, I like the implementation. But I have some issues that would need to be fixed up before I can merge it (see comments).

Concerning the lax implementation, I would say it is needed before I can release the next version. But you don't have to do it, I can also add it after I merged your PR. But let me know if you want to do it, then I don't start on it.

Greets
Julian

JulianSchmid · 2024-07-09T14:11:07Z

etherparse/src/packet_headers.rs

    /// Ethernet II header if present.
    pub link: Option<LinkHeader>,
+    /// Address Resolution Protocol if present.
+    pub arp: Option<ArpHeader>,


I would move the Arp Header into the net field as a value of the NetHeaders.

JulianSchmid · 2024-07-09T14:20:01Z

etherparse/src/link/arp_payload.rs

+
+        let src_hard_addr = HardwareAddr::new(
+            header.hw_addr_type,
+            &input[offset..(offset + header.hw_addr_size as usize)],
+        )?;


&input[offset..(offset + header.hw_addr_size as usize)] will panic if header.hw_addr_size is bigger then the slice. I would add a length check at the start that verifies the overall size of the slice:

Suggested change

let src_hard_addr = HardwareAddr::new(

header.hw_addr_type,

&input[offset..(offset + header.hw_addr_size as usize)],

)?;

if input.len() < usize::from(header.hw_addr_size)*2 + usize::from(header.proto_addr_size)*2 {

return Err(err::LenError{

// ...

});

}

let src_hard_addr = HardwareAddr::new(

header.hw_addr_type,

&input[offset..(offset + header.hw_addr_size as usize)],

)?;

JulianSchmid · 2024-07-09T14:23:05Z

etherparse/src/packet_headers.rs

            };
            len_error
        };



Can you also add arp parsing to from_ether_type?

JulianSchmid · 2024-07-09T14:26:34Z

etherparse/src/packet_headers.rs

                result.payload = payload;
            }
+            ARP => {
+                let (arp, rest) = ArpHeader::from_slice(rest).map_err(Len)?;


Suggested change

let (arp, rest) = ArpHeader::from_slice(rest).map_err(Len)?;

let (arp, rest) = ArpHeader::from_slice(rest).map_err(|err| Len(add_offset(err))?;

lieka1 · 2024-07-10T12:20:36Z

@JulianSchmid Thank you for your patients, sorry for the bug, i didn't run the test before the pr, my bad.

Of course i can do it, i feel good to do some contribution to open source

lieka1 · 2024-07-11T16:01:22Z

@JulianSchmid I am not adding any test for it, i barely understand how test is working, i think i just leave it to someone know more about it.

may be rename src/err/ip to src/err/net just to be consistant?
i also not moving src/link/arp_header.rs and src/link/arp_payload.rs to src/net

JulianSchmid · 2024-07-11T17:28:24Z

@JulianSchmid I am not adding any test for it, i barely understand how test is working, i think i just leave it to someone know more about it.

That is fine, I will have a look later if I add one.

may be rename src/err/ip to src/err/net just to be consistant?

Yes, that is fine by me. There might be some really ip specific stuff I would leave in ip. But that is not really important right now. I will have a look later, there anyways is some work that needs to be done there.

i also not moving src/link/arp_header.rs and src/link/arp_payload.rs to src/net

👍

nine-point-eight-p · 2024-10-31T08:06:57Z

Hi! I'm also interested in adding ARP to this library, and I found this PR before starting by myself. I wonder if you guys are still working on it?

JulianSchmid · 2024-11-01T17:39:09Z

Hi @nine-point-eight-p ,

I started working on it a bit more but did not finish yet. I will try to have another pass this weekend and push the result and then let you know what the state is (if I just push an update and you could have a look or if I am able to finish it).

Greets
Julian

JulianSchmid · 2024-11-11T06:12:04Z

Hi @nine-point-eight-p ,

I did further rework on the ARP implementation and pushed it to the branch arp-followup https://github.com/JulianSchmid/etherparse/tree/arp-followup . It is still not finished and I think it would need a few more passes.

I think currently it makes no sense if you help there. I still want to change a few fundamental things, but it is not concrete enough that I could write down what would be needed. It is more a "I am looking at code & change things until I am happy with it" .

But thanks again for the offer.

Greets
Julian

JulianSchmid requested changes Jul 10, 2024

View reviewed changes

JulianSchmid changed the title ~~add arp support~~ Add arp support Jan 13, 2025

JulianSchmid changed the title ~~Add arp support~~ Add ARP support Jan 13, 2025

JulianSchmid changed the title ~~Add ARP support~~ Add ARP Support Jan 13, 2025

JulianSchmid changed the title ~~Add ARP Support~~ Add ARP support Jan 13, 2025

lieka1 and others added 17 commits January 13, 2025 21:32

add arp support

d6b8709

just found out arp is not used in ipv6

2bb3c5e

fix no std

099872d

fix vlan header parse

e64eed9

move arp payload buffer to PayloadSlice

9993c88

fix bug

229cf44

bug fix, minimum test fix

5cda357

add from_ether_type arp for SlicedPackage andLaxPacketHeaders

4142059

add from_ether_type arp for LaxSlicedPacket doc minimum fix

cf44665

Further ARP refactoring

fcdfce0

Re-worked ARP implementation again

b2fb894

Resolve ARP test errors and fix bugs

f70b32b

Resolve clippy warnings and fix write bug

f99b65a

Add missing mod file

76824f0

Add arp builder methods & tests

2353094

Update documentation for ARP

a554200

Further tests & arp methods

a349dfa

JulianSchmid added 2 commits January 13, 2025 21:32

Add arp eth ipv4 specialisation

9011d72

Update Readme.md

80f5ddf

JulianSchmid force-pushed the _arp branch from c64f908 to 80f5ddf Compare January 13, 2025 20:33

JulianSchmid approved these changes Jan 13, 2025

View reviewed changes

JulianSchmid merged commit 55307c4 into JulianSchmid:master Jan 13, 2025
9 of 11 checks passed

JulianSchmid added this to the v0.17.0 milestone Jan 13, 2025

This was referenced Jan 14, 2025

Add ARP module #15

Closed

Thoughts on how to handle Arp parsing? #81

Closed

	let (arp, rest) = ArpHeader::from_slice(rest).map_err(Len)?;
	let (arp, rest) = ArpHeader::from_slice(rest).map_err(\|err\| Len(add_offset(err))?;

Add ARP support #103

Add ARP support #103

Uh oh!

Conversation

lieka1 commented Jul 2, 2024

Uh oh!

JulianSchmid commented Jul 3, 2024

Uh oh!

codecov bot commented Jul 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JulianSchmid left a comment

Choose a reason for hiding this comment

Uh oh!

JulianSchmid Jul 9, 2024

Choose a reason for hiding this comment

Uh oh!

JulianSchmid Jul 9, 2024

Choose a reason for hiding this comment

Uh oh!

JulianSchmid Jul 9, 2024

Choose a reason for hiding this comment

Uh oh!

JulianSchmid Jul 9, 2024

Choose a reason for hiding this comment

Uh oh!

lieka1 commented Jul 10, 2024

Uh oh!

lieka1 commented Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JulianSchmid commented Jul 11, 2024

Uh oh!

nine-point-eight-p commented Oct 31, 2024

Uh oh!

JulianSchmid commented Nov 1, 2024

Uh oh!

JulianSchmid commented Nov 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Jul 6, 2024 •

edited

Loading

lieka1 commented Jul 11, 2024 •

edited

Loading

JulianSchmid commented Nov 11, 2024 •

edited

Loading