Fixes and refactor

[RoDmitry]

• Fixed max size of the buffer. It can be MTU + packet information (which is u16::MAX + 4)
• Offset is not updated inside of the loop
• Store the task handle to be able to abort it
• Fixed lost packets by creating new IpStackStream if channel was closed (previously it was a trace: Send packet error: "channel closed", now it says New stream because: "channel closed")
• Refactor

[SajjadPourali]

There are some minor comments that I left; the rest looks good to me. Thanks.
I am also thinking about adding a feature for packet information to disable and enable it using that feature. What do you think?

[RoDmitry]

@SajjadPourali somehow I don't see your comments. A feature is not very friendly with understanding the code. Also once enabled, it stays enabled for all of the crates in the project. I can propose adding the const to the config. I will try doing it later. (bad idea, because MTU is not const anyway). It looks fine as it is.

[pronebird]

There are some minor comments that I left; the rest looks good to me. Thanks. I am also thinking about adding a feature for packet information to disable and enable it using that feature. What do you think?

Perhaps it would be better to use a configuration setting because the feature flag is compile time and anyone wanting to create a tun device with or without IFF_NO_PI at runtime on Linux will be at disadvantage.

[SajjadPourali]

There are some minor comments that I left; the rest looks good to me. Thanks. I am also thinking about adding a feature for packet information to disable and enable it using that feature. What do you think?

Perhaps it would be better to use a configuration setting because the feature flag is compile time and anyone wanting to create a tun device with or without IFF_NO_PI at runtime on Linux will be at disadvantage.

I agree; however, personally, I have never seen any examples where different values are passed at runtime.

[SajjadPourali]

@RoDmitry: Thanks, there were some minor code refactor comments that I addressed, one of them.

[RoDmitry]

Returning Err(e) in create_stream will break the initial tokio::select loop. I don't know if that's ok. I just saved the original logic. I've just noticed that this Result I left in create_stream was not needed. And also the Error is logged in place, if you return it, it might be logged the second time.

[SajjadPourali]

@RoDmitry It is fine to break the tokio::select as it only returns the error when the the tunnel packet sender is dropped, which means the tunnel doesn't work anymore. See https://github.com/narrowlink/ipstack/blob/main/src/stream/tcp.rs#L87

I couldn't find the place where it logs duplicates. Could you please elaborate on that?

---

I see, my bad. Let me check it one more time.

[SajjadPourali]

@RoDmitry I just made a PR to fix the issue, could you please review it?

[pronebird]

There are some minor comments that I left; the rest looks good to me. Thanks. I am also thinking about adding a feature for packet information to disable and enable it using that feature. What do you think?

Perhaps it would be better to use a configuration setting because the feature flag is compile time and anyone wanting to create a tun device with or without IFF_NO_PI at runtime on Linux will be at disadvantage.

I agree; however, personally, I have never seen any examples where different values are passed at runtime.

Yep. It's unlikely and probably impractical, although technically possible to have two tun devices, one configured with packet information, the other without.