Add packet fragmentation #1

TimonPost · 2018-10-02T18:51:51Z

This is an implementation of fragmentation (issue). Looks like it is working, but need to test it more still. So work in progress...

Note that I have looked at this libary which had some basic implementation of fragmentation. It is from the author of gafferongames blogs:

Things I changed:

There are two headers:

PacketHeader
This will contain all the header stuff we first had in RawPacket. This will be used for every packet.
Fragment Header
Fragment header the information of the fragments a packet is divided into. The first fragment packet will have two headers. The standard PacketHeader and the Fragment header the following fragments only have the fragment header.

Also, I have changed to using byteorder so that I can control more better how a header gets serialized an deserialized.
I have a FragmentBuffer which will be used to store fragments temporarily
Added a config ty[e for network settings.

Still, need to do:

Tests
Config file needs to be given by the user (for now the default is used).
I want to make the preproccess_packet function smaller it is pritty big now.

LucioFranco

This is looking great! Just a few comments. Let me know when its ready for another round.

src/lib.rs

src/net/socket_state.rs

src/net/udp.rs

LucioFranco · 2018-10-05T01:35:09Z

src/packet/header/fragment.rs

+                    wtr.write(&header.parse()?)?;
+                },
+                None => {
+                    return Err(Error::new(ErrorKind::Other, "Invalid fragment header"));


Should have an error type for this

There is one problem of doing that you have to see the context of the whole function.

Every 'write' in that function returns an io::Result. So we could paste this line .map_err(|| NetworkError::InvalidPacketHeader.into()?); behind each write function or we could simply return io::Result and let the caller map the error of that function to failiure error.

I think that is better otherwise this function will look horrible.

In reality we should have a Error::from implemented for io::Result

src/packet/header/packet.rs

src/packet/packet_processor.rs

fhaynes

Looking really good! Minor stuff from me

src/error.rs

Cargo.toml

src/lib.rs

src/net/network_config.rs

src/net/socket_state.rs

src/net/udp.rs

fhaynes · 2018-10-04T18:17:23Z

src/net/udp.rs

+        let mut bytes_send = 0;
+
+        for payload in packet_data.parts() {
+            bytes_send += self.socket.send_to(&payload, addr).map_err(|_| NetworkError::SendFailed)?;


Bubble up the error here?

src/packet/fragment_buffer.rs

fhaynes

This looks great. The one comment I made isn't a blocker, so approve.

src/error.rs

torkleyy

I didn't read the source code yet, so please forgive me for any stupid comments.

src/net/network_config.rs

torkleyy

I like it!

src/lib.rs

src/packet/header/fragment.rs

…into fragmentation

LucioFranco

This is looking really good! Just a few small nits. Once, those are resolved and you resolve @torkleyy's comments. Squash this into one commit, with a description and then merge it!

LucioFranco · 2018-10-06T22:19:06Z

src/error.rs


 pub type Error = failure::Error;
 pub type Result<T> = result::Result<T, Error>;

+


Remove these new lines please

src/packet/fragment_buffer.rs

fhaynes · 2018-10-07T00:25:19Z

@TimonPost I took a stab at resolving the conflicts

LucioFranco · 2018-10-07T02:50:36Z

@fhaynes btw

error: expected `;`, found `use`
 --> src/net/udp.rs:5:1
  |
4 | use super::{constants, Packet, RawPacket}
  |                                          - expected `;`
5 | use socket_state::SocketState; 
  | ^^^ unexpected token
```

Fixed a missing semicolon

fhaynes · 2018-10-07T02:54:40Z

@fhaynes btw

error: expected `;`, found `use`
 --> src/net/udp.rs:5:1
  |
4 | use super::{constants, Packet, RawPacket}
  |                                          - expected `;`
5 | use socket_state::SocketState; 
  | ^^^ unexpected token

Fixed, thanks.

TimonPost · 2018-10-07T08:30:10Z

Oke fixed the merge, ran all the tests with success, removed some unused imports and fixed all your comments. So going to merge now since you both have approved.

…into fragmentation

Update from upstream

TimonPost added 2 commits October 2, 2018 20:50

implemented fragmentation

c1ee1b0

refactors

cb7e472

TimonPost mentioned this pull request Oct 2, 2018

WIP: Packet Fragmentation implementation | DO NOT MERGE LucioFranco/amethyst-protocol#22

Closed

added tests

f80983a

LucioFranco changed the title ~~implemented fragmentation~~ Add packet fragmentation Oct 4, 2018

LucioFranco requested review from LucioFranco and fhaynes October 4, 2018 22:09

LucioFranco suggested changes Oct 5, 2018

View reviewed changes

refactors, maked packet and headers read only after declaration

ac3eb8f

fhaynes reviewed Oct 5, 2018

View reviewed changes

fhaynes approved these changes Oct 5, 2018

View reviewed changes

src/error.rs Outdated Show resolved Hide resolved

TimonPost added 2 commits October 6, 2018 00:19

Removed commented code

f13490f

PR improvements

b48d703

torkleyy reviewed Oct 6, 2018

View reviewed changes

src/net/network_config.rs Outdated Show resolved Hide resolved

src/net/network_config.rs Show resolved Hide resolved

torkleyy reviewed Oct 6, 2018

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

src/lib.rs Show resolved Hide resolved

src/packet/header/fragment.rs Outdated Show resolved Hide resolved

TimonPost added 2 commits October 6, 2018 17:03

grammer fix, comment fix

b0e6cf0

Merge branch 'fragmentation' of https://github.com/TimonPost/laminar …

8fa85ac

…into fragmentation

LucioFranco approved these changes Oct 6, 2018

View reviewed changes

Merge branch 'master' into fragmentation

143257d

Update udp.rs

25ae3c0

Fixed a missing semicolon

TimonPost merged commit 69cd007 into TimonPost:master Oct 7, 2018

TimonPost added 2 commits October 7, 2018 12:02

Some PR comment updates

6bdbc2b

Merge branch 'fragmentation' of https://github.com/TimonPost/laminar …

5542a61

…into fragmentation

TimonPost mentioned this pull request Oct 7, 2018

Implement fragmentation #2

Closed

fixed PR comments, fixed errors from merge, removed unused imports

e020d7b

TimonPost deleted the fragmentation branch October 9, 2018 10:18

TimonPost mentioned this pull request Oct 22, 2018

Milestone 0.1.0 #58

Closed

18 tasks

fhaynes added a commit that referenced this pull request Oct 28, 2018

Merge pull request #1 from amethyst/master

1769c42

Update from upstream

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add packet fragmentation #1

Add packet fragmentation #1

TimonPost commented Oct 2, 2018 •

edited

Loading

LucioFranco left a comment

LucioFranco Oct 5, 2018

TimonPost Oct 5, 2018 •

edited

Loading

LucioFranco Oct 5, 2018

fhaynes left a comment

fhaynes Oct 4, 2018

fhaynes left a comment

torkleyy left a comment

torkleyy left a comment

LucioFranco left a comment

LucioFranco Oct 6, 2018

fhaynes commented Oct 7, 2018

LucioFranco commented Oct 7, 2018

fhaynes commented Oct 7, 2018

TimonPost commented Oct 7, 2018


		pub type Error = failure::Error;
		pub type Result<T> = result::Result<T, Error>;

Add packet fragmentation #1

Add packet fragmentation #1

Conversation

TimonPost commented Oct 2, 2018 • edited Loading

LucioFranco left a comment

Choose a reason for hiding this comment

LucioFranco Oct 5, 2018

Choose a reason for hiding this comment

TimonPost Oct 5, 2018 • edited Loading

Choose a reason for hiding this comment

LucioFranco Oct 5, 2018

Choose a reason for hiding this comment

fhaynes left a comment

Choose a reason for hiding this comment

fhaynes Oct 4, 2018

Choose a reason for hiding this comment

fhaynes left a comment

Choose a reason for hiding this comment

torkleyy left a comment

Choose a reason for hiding this comment

torkleyy left a comment

Choose a reason for hiding this comment

LucioFranco left a comment

Choose a reason for hiding this comment

LucioFranco Oct 6, 2018

Choose a reason for hiding this comment

fhaynes commented Oct 7, 2018

LucioFranco commented Oct 7, 2018

fhaynes commented Oct 7, 2018

TimonPost commented Oct 7, 2018

TimonPost commented Oct 2, 2018 •

edited

Loading

TimonPost Oct 5, 2018 •

edited

Loading