Skip to content

Updates based on feedback on #1222 and more#1280

Merged
fwsGonzo merged 46 commits intoincludeos:devfrom
AnnikaH:dev
Apr 6, 2017
Merged

Updates based on feedback on #1222 and more#1280
fwsGonzo merged 46 commits intoincludeos:devfrom
AnnikaH:dev

Conversation

@AnnikaH
Copy link
Copy Markdown
Member

@AnnikaH AnnikaH commented Mar 30, 2017

  • Updates based on feedback on pull request Statman and ICMP #1222
  • Socket moved out of namespace tcp
  • Error handling with net::Error (and net::ICMP_error)
  • Handling of Error callbacks in UDP
  • DNS integration test
    and more

AnnikaH added 30 commits March 21, 2017 16:42
…terator to end of span, not last Stat-element (same as end)
…ing and ICMP_packet now has a payload of type string instead of Span
…ror_report signature. Added timer that flushes all sendto_handler callbacks that have not resulted in an error (ICMP_error f.ex.)
…on. Constructor taking an icmp4 Packet (cleaner)
…an ICMP packet) that works on both Mac and Ubuntu (the previous one doesn't work on Ubuntu)
… ping with DNS resolution (in progress). Cleanup
AnnikaH added 9 commits March 30, 2017 13:26
…ing slicing) and no longer const when passed between functions. DNS integration test updated to test this (dynamic cast from Error to ICMP_error)
…dress ending with .255 does not have to be a brodcast address). Updated test for whether IP address is multicast or broadcast in IP4 and UDP. IP4 unit test update: IP packet with unknown protocol gets dropped
…to Socket (moved to namespace net) and DNS resolution (callback with Error)
…callback and updates in that regard elsewhere in the OS. UDP: Socket is now key into error_callbacks_ map. Socket: Added hash function so that this can be a key into a map
Copy link
Copy Markdown
Contributor

@AndreasAakesson AndreasAakesson left a comment

Choose a reason for hiding this comment

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

Looking good 👍

Comment thread api/net/inet.hpp

template <typename IPv>
using resolve_func = delegate<void(typename IPv::addr)>;
using resolve_func = delegate<void(typename IPv::addr, Error&)>;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Question: Are there use cases where Error is modified inside the callback? If not, maybe pass it as const reference?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No seems like Error& can be const here =)

Comment thread api/net/ip4/udp.hpp
/** Error entries are just error callbacks and timestamps */
class Error_entry {
public:
Error_entry(UDP::error_handler cb) noexcept
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

By moving the implementation of the constructor to the cpp you can avoid including <rtc> in the header (reducing the scope).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

private attribute RTC::timestamp_t, but could consider int64_t

Comment thread api/net/ip4/udp.hpp
}; //< class Error_entry

/** The error callbacks that the user has sent in via the UDPSockets' sendto and bcast methods */
std::unordered_map<Socket, Error_entry, Socket::pair_hash> error_callbacks_;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A implementation of std::hash<Socket> would make it possible to drop the third template arg Socket::pair_hash (this function can be called inside the std::hash`). See http://en.cppreference.com/w/cpp/utility/hash

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

👍

Comment thread api/net/socket.hpp
: source_{}, destination_{}
{}

Quadruple(const Socket::Address src_address, const Socket::port_t src_port,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe add additional constructor Quadruple(Socket, Socket).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

👍

@alfreb
Copy link
Copy Markdown
Contributor

alfreb commented Apr 5, 2017

jenkins test please

@mnordsletten
Copy link
Copy Markdown
Contributor

This is the error I was seeing yesterday as well @AnnikaH where it seems like you were not expecting the 10.0.0.1 dns server to actually give a valid response. (but it seems like it does)

Resolved IP address of hotmail.com with DNS server 10.0.0.1: 157.56.198.220

@fwsGonzo fwsGonzo merged commit c93832f into includeos:dev Apr 6, 2017
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.

5 participants