Statman and ICMP by AnnikaH · Pull Request #1222 · includeos/IncludeOS

AnnikaH · 2017-03-06T18:18:41Z

No description provided.

…d and working on other ICMP messages

…eation of Stat throws Exception, empty name-input to get-method now throws immediate Exception

…(RFC 1122). Removed deprecated ICMP messages (Source Quench and Information - RFC 6918 and 6633)

…nstead of previously updated statman_.end() iterator

…so that UDP and IP4 have access to them

…MP messages

…messages)

….hpp

…mp4::Packet

…ence

…ed message, port and protocol unreachable validated in test.py

…nding to incoming ping requests

… span. Get(name) method using last_used-iterator instead of end

… on linux for now)

…moving them on timeout), updating integration test correspondingly and setting timeout to 40 (reply) and 50 (test)

AnnikaH · 2017-03-10T15:34:58Z

Jenkins test please

… exceeded messages implemented

…need for IP forwarding)

…heck on transmitted packets as well

…ng error CPU model host requires KVM

…with dns set to 10.0.0.1 changed to 8.8.8.8

…be undone later

… ICMP_packet and small cleanups

…d for is_reply()

…ot broadcast or multicast (RFC 1122)

AnnikaH · 2017-03-20T11:40:56Z

Jenkins test please

AndreasAakesson

Looking good 👍 Just some minor stuff/improvements

AndreasAakesson · 2017-03-20T11:49:08Z

+     *  Error report in accordance with RFC 1122
+     *  An ICMP error message has been received - forward to transport layer (UDP or TCP)
+    */
+    void error_report(Error_type type, Error_code code, Span icmp_payload) override {


Can this be put in .cpp?

AndreasAakesson · 2017-03-20T11:53:03Z

+      ICMP_callback(ICMPv4& icmp, Tuple t, icmp_func cb)
+      : tuple{t}, callback{cb}
+      {
+        timer_id = Timers::oneshot(std::chrono::seconds(40), [&icmp, t](Timers::id_t) {


Maybe have the timeout duration configurable (like passed into the origin call that creates a ICMP callback)?
Or at least define the timeout duration static outside the callback struct, and have it globally configurable.

Would make sense as part of ping(...)

AndreasAakesson · 2017-03-20T12:02:00Z

+      auto it = ping_callbacks_.find(std::make_pair(ping_response.id(), ping_response.sequence()));
+
+      if (it != ping_callbacks_.end()) {
+        it->second.callback(ICMP_packet{ping_response.id(), ping_response.sequence(), ping_response.ip().ip_src(),


To make it cleaner you could add a constructor to struct ICMP_packet that takes a const icmp4::Packet& and hide all these lines there (with a shorter name).

AndreasAakesson · 2017-03-20T12:04:21Z

+  } // < namespace code
+
+  static std::string __attribute__((unused)) get_type_string(Type type) {
+    return [&type] () {


This lambda doesn't fill any purpose here.

AndreasAakesson · 2017-03-20T12:07:00Z

+  }
+
+  static std::string __attribute__((unused)) get_code_string(Type type, uint8_t code) {
+    return [&type, &code] () {


Same as above.
Maybe have helper functions for the nested switch cases? It also could make sense to move this to a cpp file.

AndreasAakesson · 2017-03-20T12:13:10Z

+  }
+
+  void ICMPv4::destination_unreachable(Packet_ptr pckt, icmp4::code::Dest_unreachable code) {
+    if ((size_t)pckt->size() < sizeof(IP4::header) + icmp4::Packet::header_size()) // Drop if not a full header


You could make it more DRY by creating a inline helper function for this, and also use UNLIKELY:
if(UNLIKELY(not full_header(*pckt))) return;
(Could this be done before any of the functions are called?)

alfreb · 2017-03-20T12:12:22Z

+
+  void ICMPv4::forward_to_transport_layer(icmp4::Packet& req) {
+    // inet forwards to transport layer (UDP or TCP)
+    inet_.error_report(req.type(), req.code(), req.payload());


I suggest passing the underlying packet as IP packet pointer to retain storage of the underlying buffer. e.g.

auto packet_ptr = req.release(); packet_ptr -> set_layer_begin(req.payload()); // hide information below payload, interpret the rest as Packet_IP4 inet_.error_report(type, code, std::move(packet_ptr));

alfreb · 2017-03-20T12:16:24Z

+      {
+        stack_.icmp().destination_unreachable(std::move(packet), icmp4::code::Dest_unreachable::PROTOCOL);
+      }
+


alfreb · 2017-03-20T12:17:21Z

+    TRAILER_NEGO  = 0x0010,   // RFC 893 Trailer negotiation
+    TRAILER_FIRST = 0x0110,   // RFC 893, 1122 Trailer encapsulation
+    TRAILER_LAST  = 0x0f10
  };


alfreb · 2017-03-20T12:22:09Z

+    operator bool() const noexcept
+    { return type_ != icmp4::Type::NO_REPLY; }
+
+    std::string to_string();


alfreb · 2017-03-20T12:27:05Z

+      ICMP_callback(ICMPv4& icmp, Tuple t, icmp_func cb)
+      : tuple{t}, callback{cb}
+      {
+        timer_id = Timers::oneshot(std::chrono::seconds(40), [&icmp, t](Timers::id_t) {


Would make sense as part of ping(...)

alfreb · 2017-03-20T12:49:49Z

  netmask_{stack_.netmask()},
  router_{stack_.gateway()},
-  dns_{stack_.gateway()},
+  dns_{stack_.dns()},


If there's a getter for both the DNS client and the DNS address, this is ambiguous

For IP I think we used ip_obj and ip_addr

alfreb · 2017-03-20T12:51:35Z

+      (t >= net::ntohs(static_cast<uint16_t>(Ethertype::TRAILER_FIRST)) and
+        t <= net::ntohs(static_cast<uint16_t>(Ethertype::TRAILER_LAST))))) {
+      debug("<Ethernet OUT> Ethernet type Trailer is not supported. Packet is not transmitted\n");
+      return;


👍 (we might want to add TX_dropped as either delegate or just stat - but we can do that later)

alfreb · 2017-03-20T13:03:15Z

+    }
+  }
+
+  void UDP::error_report(Error_type type, Error_code code,


Signature will change to e.g. UDP::error_report(ICMP_Error) or something, as discussed

alfreb · 2017-03-20T13:04:34Z

  return ss.str();
 }

+void TCP::error_report(Error_type type, Error_code code,


New signature as discussed for UDP

alfreb · 2017-03-20T13:07:20Z

+  });
+
+  // Also possible without callback:
+  // inet.icmp().ping(IP4::addr{193,90,147,109});  // google.com


Would also be nice to do DNS resolution - so you could ping("google.com", callback). For later.

…ad of Span (move away from raw pointers)

Updates based on feedback on #1222 and more

AnnikaH added 25 commits March 3, 2017 16:27

ICMP: Renamed icmpv4.hpp/cpp -> icmp4.hpp/cpp Ping functionality adde…

32b081e

…d and working on other ICMP messages

Statman: End-iterators now correct, next_available_ now correct if cr…

f2365b9

…eation of Stat throws Exception, empty name-input to get-method now throws immediate Exception

ICMP4: Added RFC 1122 codes

bc6b61b

Merge branch 'dev' of github.com:hioa-cs/IncludeOS into dev

0304287

ICMP: Added more Destination Unreachable and Parameter Problem codes …

6252d01

…(RFC 1122). Removed deprecated ICMP messages (Source Quench and Information - RFC 6918 and 6633)

Test CMakeLists.txt: icmpv4.cpp -> icmp4.cpp

e4dc73d

Statman unit test: Filling statman with stats using statman_.size() i…

2b920fd

…nstead of previously updated statman_.end() iterator

Merge branch 'dev' of github.com:hioa-cs/IncludeOS into dev

392499c

Inet: Getter for ICMPv4 object

ce3fc68

Inet common and packet_icmp4: Moved icmp4 codes into inet_common.hpp …

ef91337

…so that UDP and IP4 have access to them

IP4 Packet: Getter for TTL added

bc9c5a1

ICMP4: Support for sending ping, destination unreachable and other IC…

235ae06

…MP messages

IP4 and UDP: Sending protocol unreachable and port unreachable (ICMP …

6f08c0d

…messages)

ICMP integration test: Files added

001563a

ICMP integration test: Added vm.exit to test.py

97b81b3

ICMP4 codes separated into its own file and included into inet_common…

bfc797a

….hpp

ICMP4: Added method for getting destination unreachable payload in ic…

9b6542d

…mp4::Packet

ICMP4: Ping with callback, send response with and without id and sequ…

be46d50

…ence

ICMP integration test: ping with callback to validate sent and receiv…

f7c75e6

…ed message, port and protocol unreachable validated in test.py

Merge branch 'dev' of github.com:hioa-cs/IncludeOS into dev

6ba1e09

ICMP4 ping reply: Calling new method send_response_with_id when respo…

cf29a15

…nding to incoming ping requests

ICMP integration test: Printing out result from ping callback

3a77b84

Statman: end-iterator back to returning end of span, not last stat in…

bfb84da

… span. Get(name) method using last_used-iterator instead of end

ICMP integration test: Marked as intrusive and installing hping (just…

b4b4301

… on linux for now)

ICMP4 incl. integration test: Complete handling of ping-callbacks (re…

7552954

…moving them on timeout), updating integration test correspondingly and setting timeout to 40 (reply) and 50 (test)

AnnikaH added 4 commits March 10, 2017 16:48

ICMP integration test: -y added to install hping3

54d7574

ICMP4: Parameter problem (with pointer indicating the error) and time…

df60f52

… exceeded messages implemented

ICMP4: got_reply_ -> is_reply_, integration test pinging gateway (no …

d8360bf

…need for IP forwarding)

Ethernet: Trailer nego and encapsulation (ethertype) added with counter

92c5f80

AnnikaH added 14 commits March 14, 2017 14:44

ICMP_packet: to_string implementation moved to cpp file

aa4cd69

Ethernet: Trailer encapsulation now taking byte order into account. C…

6805695

…heck on transmitted packets as well

Merge branch 'dev' of github.com:hioa-cs/IncludeOS into dev

9d8465e

Test vm.json: Removed cpu host to be able to run boot . without getti…

1f0d980

…ng error CPU model host requires KVM

Merge branch 'dev' of github.com:hioa-cs/IncludeOS into dev

54cb698

ICMP4: printf -> debug

555046a

ICMP forwarding error messages to UDP and TCP through Inet

523b84d

Merge branch 'dev' of github.com:AnnikaH/IncludeOS into dev

9f99b9e

Merge branch 'dev' of github.com:hioa-cs/IncludeOS into dev

f2d4863

Inet getter for dns added and DHCPD (server) now uses this. Services …

82f84d7

…with dns set to 10.0.0.1 changed to 8.8.8.8

Testing: Commented out tar unittests to check Jenkins' reaction - to …

a74b134

…be undone later

ICMP integration test: Update test to reflect new to_string method in…

733162a

… ICMP_packet and small cleanups

ICMP_packet and integration test update: Added bool operator - no nee…

aea8c7f

…d for is_reply()

IP4 and UDP sending ICMP dest unreachable: Only when destination is n…

7cc24cb

…ot broadcast or multicast (RFC 1122)

AndreasAakesson reviewed Mar 20, 2017

View reviewed changes

alfreb reviewed Mar 20, 2017

View reviewed changes

AnnikaH added 7 commits March 20, 2017 14:19

Merge branch 'dev' of github.com:hioa-cs/IncludeOS into dev

ac5b0c6

Rename back: icmp4 -> icmpv4 to later do git move on this change instead

72cfe98

Git mv icmpv4 -> icmp4 to keep history

bfb7f76

Inet, Inet4, ICMP4, PacketICMP4: error_report taking Packet_ptr inste…

69b0ec3

…ad of Span (move away from raw pointers)

Merge branch 'dev' of github.com:hioa-cs/IncludeOS into dev

13d64f6

Ethernet: Merge fix (variable name miss)

ea4ca76

Merge branch 'dev' of github.com:hioa-cs/IncludeOS into dev

aab718b

alfreb merged commit 6180dc9 into includeos:dev Mar 21, 2017

AnnikaH mentioned this pull request Mar 30, 2017

Updates based on feedback on #1222 and more #1280

Merged

fwsGonzo added a commit that referenced this pull request Apr 6, 2017

Merge pull request #1280 from AnnikaH/dev

c93832f

Updates based on feedback on #1222 and more

Conversation

AnnikaH commented Mar 6, 2017

Uh oh!

AnnikaH commented Mar 10, 2017

Uh oh!

AnnikaH commented Mar 20, 2017

Uh oh!

AndreasAakesson left a comment

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants