refactor codec.rs #2

NamsooCho · 2018-01-25T09:35:48Z

Refactor codec.rs for UDP packet.

coveralls · 2018-01-25T09:54:04Z

Pull Request Test Coverage Report for Build 98

134 of 217 (61.75%) changed or added relevant lines in 4 files are covered.
4 unchanged lines in 4 files lost coverage.
Overall coverage decreased (-1.06%) to 96.104%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/toxcore/dht_new/packet_kind.rs	1	7	14.29%
src/toxcore/dht_new/codec.rs	25	49	51.02%
src/toxcore/dht_new/packet.rs	73	126	57.94%

Files with Coverage Reduction	New Missed Lines	%
src/toxcore/dht_new/kbucket.rs	1	98.54%
src/toxcore/dht_new/packed_node.rs	1	66.04%
src/toxcore/dht_new/packet.rs	1	84.99%
src/toxcore/dht.rs	1	98.73%

Totals
Change from base Build 96:	-1.06%
Covered Lines:	6142
Relevant Lines:	6391

💛 - Coveralls

kpp · 2018-01-25T11:08:29Z

src/toxcore/dht_new/dht_impl.rs

+                    Ok(None)
+                }
+            },
+            _ => Ok(None)


add log! or smt like this

Good catch!

kpp · 2018-01-25T11:12:45Z

src/toxcore/dht_new/dht_impl.rs

+    */
+    // Because UDP codec and tokio use DhtBase for send/receive packet, 
+    // this function returns DhtBase type object
+    pub fn ping_response(&self,


I think this method should be located inside DHT Node but not inside DhtPacket

I agree.
I moved this function to DhtPacket.rs

kpp · 2018-01-25T11:20:35Z

src/toxcore/dht_new/dht_impl.rs

+    }
+}
+
+impl GetNodes {


I was reading DhtRequest and then appeared GetNodes. It is unexpected

Do you mean that the appearing order is not good?
Or impl GetNodes{...} should be other file?

Well, you may either move it to change the order or move it to another file.

Now, dht_impl.rs has collection of impl of Types.
This concept should be discussed more.

kpp · 2018-01-26T08:57:16Z

src/toxcore/dht_new/dht_impl.rs

+    }
+
+    /// Serialize Public, Secret Keys to u8 array
+    unsafe fn any_as_u8_slice<T: Sized>(p: &T) -> &[u8] {


I don't like the solution to keep PrecomputedKey in the cache as ... serialized slice. But keeping PrecomputedKey in the cache is OK!

I agree.
Now, the hasher for SecrectKey, PublicKey, PrecomputedKey is not provided by sodium-oxide.
I am trying to implement custom Hasher.

kpp · 2018-01-26T09:01:30Z

src/toxcore/dht_new/dht_impl.rs

+            Ok(d) => d,
+            Err(_) => {
+                debug!("Decrypting DhtPacket failed!");
+                return Ok(None);


I don't think that Ok(None) is a good idea.

I agree.
If the result of this function is Err, it should contain Err info.
I will change this.

kurnevsky · 2018-01-28T20:51:41Z

src/toxcore/dht_new/codec.rs

+pub type DhtRecvUdpPacket = (SocketAddr, Option<DhtBase>);
+
+/**
+SendNodes


I think it should be explained better why we use SendNodes here to estimate maximum size of DhtPacket.

Good catch!

kurnevsky · 2018-01-28T20:52:07Z

src/toxcore/dht_new/dht_impl.rs

+        let shared_secret = &encrypt_precompute(key_pair.1, key_pair.0);
+        let shared_arr = unsafe { PrecomputedKeys::any_as_u8_slice(&shared_secret) };
+        self.cache.insert (key.clone(), shared_arr.to_vec());
+        return Some(shared_secret.clone());


No need to use return here.

Good catch!

kurnevsky · 2018-01-28T20:52:49Z

src/toxcore/dht_new/dht_impl.rs

+            Some(k) => {
+                return PrecomputedKey::from_slice(k);
+            },
+            None => { ; },


Just { } should be enough.

Good catch!

kurnevsky · 2018-01-30T08:36:22Z

src/toxcore/dht_new/dht_impl.rs

+
+    /// Get precomputed keys
+    /// If the Key is not found in cache, create symmetric key and insert it into cache for later use.
+    pub fn get_symmetric_key (&mut self, key_pair: (&SecretKey, &PublicKey)) -> Option<PrecomputedKey> {


Why Option?

I will change return type from Option to Result to handle error better.

kpp · 2018-01-31T04:45:20Z

src/toxcore/dht_new/packet_kind.rs

+*/
+impl FromBytes for PacketKind {
+    named!(from_bytes<PacketKind>, switch!(le_u8,
+    0   => value!(PacketKind::PingReq) |


kpp · 2018-01-31T22:47:55Z

Cargo.toml

 tokio-io = "0.1"
 nom = "3.2"
 cookie-factory = "0.2.2"
+bincode = "0.9.2"


Do we need bincode?

kpp · 2018-01-31T22:48:31Z

src/toxcore/dht_new/codec.rs

+                Ok((*src, None))
+            },
+            IResult::Error(_) => {
+                Ok((*src, None))


kpp · 2018-01-31T23:00:08Z

src/toxcore/dht_new/codec.rs

+    fn encode(&mut self, (addr, dp): Self::Out, into: &mut Vec<u8>) -> SocketAddr {
+        let mut buf = [0; MAX_DHT_PACKET_SIZE];
+        if let Ok((_, size)) = dp.to_bytes((&mut buf, 0)) {
+                into.extend(&buf[..size]);


kpp · 2018-02-01T00:10:37Z

src/toxcore/dht_new/codec.rs

-                into.extend(&buf[..size]);
+            into.extend(&buf[..size]);
+        // TODO: move from tokio-core to tokio and return error instead of panic
        } else {panic!("DhtBase to_bytes error {:?}", dp);}


Anyway you have to fix indent

It is fixed

TODO is about panic!, So It's indent is adjusted to else {

kurnevsky · 2018-02-01T09:21:22Z

src/toxcore/dht_new/packet.rs

+                    DhtBase::DhtPacket(DhtPacket::arbitrary(g)),
+                1 =>
+                    DhtBase::DhtRequest(DhtRequest::arbitrary(g)),
+                _ => panic!("Arbitrary for DhtBase - should not have happened!")


There is unreachable! macro for such cases.

if choice == 0 {} else {} will work too.

kpp · 2018-02-01T12:19:52Z

src/toxcore/dht_new/packet.rs

+                    DhtPacket::new(&precomputed, &pk, DhtPacketPayload::GetNodes(GetNodes::arbitrary(g))),
+                3 =>
+                    DhtPacket::new(&precomputed, &pk, DhtPacketPayload::SendNodes(SendNodes::arbitrary(g))),
+                _ => panic!("Arbitrary for DhtPacket - should not have happened!")


unreachable!

kurnevsky · 2018-02-05T11:28:30Z

src/toxcore/dht_new/codec.rs

+use std::io::{Error, ErrorKind};
+use tokio_core::net::UdpCodec;
+use std::net::SocketAddr;
+//use std::io::{Error, ErrorKind};


Already imported, so remove.

kurnevsky · 2018-02-05T11:30:30Z

src/toxcore/dht_new/packet.rs

+                let ping_kind = match stringify!($np) {
+                    "NatPingRequest" => NAT_PING_REQUEST as u8,
+                    "NatPingResponse" => NAT_PING_RESPONSE as u8,
+                    e => panic!("can not occur {:?}", e)


unreachable!

kurnevsky · 2018-02-05T13:27:14Z

src/toxcore/dht_new/packet.rs

+impl ToBytes for DhtRequest {
+    fn to_bytes<'a>(&self, buf: (&'a mut [u8], usize)) -> Result<(&'a mut [u8], usize), GenError> {
+        do_gen!(buf,
+            gen_be_u8!(0x20) >>


Maybe use PacketKind::DhtRequest as u8?

Let it be 0x20

DHT server initial implementation

kpp reviewed Jan 25, 2018

View reviewed changes

kpp reviewed Jan 26, 2018

View reviewed changes

kurnevsky reviewed Jan 28, 2018

View reviewed changes

kurnevsky reviewed Jan 30, 2018

View reviewed changes

kpp reviewed Jan 31, 2018

View reviewed changes

Cargo.toml Outdated

tokio-io = "0.1"

nom = "3.2"

cookie-factory = "0.2.2"

bincode = "0.9.2"

Copy link

Member

kpp Jan 31, 2018

Choose a reason for hiding this comment

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

Do we need bincode?

kpp reviewed Jan 31, 2018

View reviewed changes

src/toxcore/dht_new/codec.rs Outdated

Ok((*src, None))

},

IResult::Error(_) => {

Ok((*src, None))

Copy link

Member

kpp Jan 31, 2018

Choose a reason for hiding this comment

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

Why Ok?

kpp reviewed Jan 31, 2018

View reviewed changes

kpp reviewed Feb 1, 2018

View reviewed changes

kurnevsky reviewed Feb 1, 2018

View reviewed changes

kpp reviewed Feb 1, 2018

View reviewed changes

NamsooCho changed the title ~~refactor(dht): codec & dht_node~~ refactor codec.rs Feb 2, 2018

kpp requested review from kpp and kurnevsky February 3, 2018 21:19

kpp approved these changes Feb 5, 2018

View reviewed changes

kurnevsky reviewed Feb 5, 2018

View reviewed changes

kurnevsky approved these changes Feb 5, 2018

View reviewed changes

NamsooCho added 5 commits February 7, 2018 07:33

refactor(dht): remove pk() function

703ea3d

refactor(dht): include codec, packet_kind

4134791

refactor(dht): remove pk() function

6d39241

refactor(dht): introduce DhtBase enum

f172480

refactor(dht): add codec.rs

afbaddc

NamsooCho added 3 commits February 7, 2018 07:33

refactor(dht): add packet_kind.rs

8fa1813

refactor(dht): Remove dht_new binary_io in favor of binary_io_new

488f60c

refactor(dht): change from panic! to unreachable!

52b84f2

kpp merged commit 9388a7e into tox-rs:master Feb 7, 2018

NamsooCho deleted the codec branch February 7, 2018 01:12

NamsooCho restored the codec branch February 7, 2018 01:13

NamsooCho deleted the codec branch February 7, 2018 01:14

kurnevsky pushed a commit that referenced this pull request Sep 4, 2022

Merge pull request #2 from tox-rs/dht_server

2276eab

DHT server initial implementation

refactor codec.rs #2

refactor codec.rs #2

Uh oh!

Conversation

NamsooCho commented Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 98

💛 - Coveralls

Uh oh!

kpp Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NamsooCho Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kpp Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kpp Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NamsooCho commented Jan 25, 2018 •

edited

Loading

coveralls commented Jan 25, 2018 •

edited

Loading

kpp Jan 25, 2018 •

edited

Loading

NamsooCho Jan 25, 2018 •

edited

Loading

kpp Jan 26, 2018 •

edited

Loading

kpp Jan 31, 2018 •

edited

Loading