Skip to content

Conversation

@NamsooCho
Copy link
Member

@NamsooCho NamsooCho commented Feb 8, 2018

apply new DhtPacketPayload::from_bytes() to get_payload() of dht_impl.rs.
This PR has rebased on #11

@coveralls
Copy link

coveralls commented Feb 8, 2018

Pull Request Test Coverage Report for Build 119

  • 4 of 31 (12.9%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 95.698%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/toxcore/dht_new/packet.rs 4 31 12.9%
Totals Coverage Status
Change from base Build 99: 0.0%
Covered Lines: 6139
Relevant Lines: 6415

💛 - Coveralls

// TODO: add fn for ping/getn req timeouts with hardcoded consts?
pub fn remove_timed_out(&mut self, secs: u64) {
for pk in self.getn_timeout.get_timed_out(secs) {
debug!("Removing timed out node");
Copy link
Member

Choose a reason for hiding this comment

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

I think pk should be added to the message, in other case it will be useless to have many same records in a row.

Copy link
Member

@kpp kpp Feb 8, 2018

Choose a reason for hiding this comment

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

We should remove that code by Zetok. It is malformed.

trace!(target: "DhtPacket", "With DhtPacket: {:?}", self);
let decrypted = open(&self.payload, &self.nonce, &self.pk,
own_secret_key)
.and_then(|d| Ok(d))
Copy link
Member

Choose a reason for hiding this comment

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

It seems this line is unnecessary.

format!("DhtPacket decrypt error: {:?}", e))
});

match DhtPacketPayload::from_bytes(&decrypted.unwrap_or(vec![0]), self.packet_kind) {
Copy link
Member

@kurnevsky kurnevsky Feb 8, 2018

Choose a reason for hiding this comment

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

Why unwrap_or(vec![0])? You already did .map_err(..) part so just add question mark to return error properly.

// 1 | nat ping req|resp (0xfe)
// 1 | Request or Response flag
// 8 | Request Id (Ping Id)
let packet_payload = decrypted.unwrap_or(vec![0,0]);
Copy link
Member

Choose a reason for hiding this comment

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

I'd propose to do here the same thing as for DhtPacket::get_payload to reduce code size.

if let DhtBase::DhtPacket(ref p) = *self {
p.packet_kind
} else {
unreachable!("DhtRequest packet kind is encrypted.")
Copy link
Member

@kurnevsky kurnevsky Feb 8, 2018

Choose a reason for hiding this comment

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

Why this is unreachable? And why error instead of returning PacketKind::DhtRequest?

let key = HashKeys(key_pair.0.clone(), key_pair.1.clone());
match self.cache.get(&key) { // if symmetric key exists in cache, returns with the value
Some(k) => {
return Ok(k.clone());
Copy link
Member

Choose a reason for hiding this comment

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

Space at the end of line :)

@NamsooCho NamsooCho closed this Feb 8, 2018
@NamsooCho NamsooCho deleted the codec branch February 8, 2018 22:50
@NamsooCho
Copy link
Member Author

I reopen this PR with new contents.
New PR has only codec related codes.

@NamsooCho NamsooCho restored the codec branch February 8, 2018 23:27
@NamsooCho NamsooCho reopened this Feb 8, 2018
@kpp kpp changed the title apply new DhtPacketPayload::from_bytes Add DhtPacket::get_payload Feb 8, 2018
format!("DhtPacket decrypt error: {:?}", e))
});

match DhtPacketPayload::from_bytes(&decrypted.unwrap_or(vec![0]), self.packet_kind) {
Copy link
Member

Choose a reason for hiding this comment

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

Why unwrap_or(vec![0])? You already did .map_err(..) part so just add question mark to return error properly.

trace!(target: "DhtPacket", "With DhtPacket: {:?}", self);
let decrypted = open(&self.payload, &self.nonce, &self.pk,
own_secret_key)
.and_then(|d| Ok(d))
Copy link
Member

Choose a reason for hiding this comment

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

It seems this line is unnecessary.

- fails to decrypt
- fails to parse as given packet type
*/
pub fn get_payload(&self, own_secret_key: &SecretKey) -> Result<Option<DhtPacketPayload>, Error>
Copy link
Member

Choose a reason for hiding this comment

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

Why Option here?

@kpp kpp merged commit 30dbd09 into tox-rs:master Feb 9, 2018
kurnevsky pushed a commit that referenced this pull request Sep 4, 2022
Make TCP and UDP servers optional
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants