Support ICMP Time Exceeded Message #issues57 #274

darkgrey1554 · 2025-02-07T10:41:42Z

No description provided.

dataplane/worker.cpp

autotest/units/001_one_port/081_time_exceeded_in_transit/gen.py

...s/001_one_port/081_time_exceeded_in_transit/controlplane_full_config_hidden_ip_disabled.conf

dataplane/icmp.cpp

dataplane/worker.cpp

ol-imorozko · 2025-02-10T13:25:50Z

Add support ICMP Time Exceeded Message is grammatically incorrect. I suggest to just use Support ICMP Time Exceeded Message.
Also it would be really helpful if you add commit description about why we need this. You can just copy issue description and have smth like:

Support ICMP Time Exceeded Message

YANET doesn't support ICMP response, when receiving packet with TTL = 1.
By RFC792 YANET should send ICMP Time Exceeded Message,
when packet cannot be forwarded or balanced.

ol-imorozko · 2025-02-10T13:30:07Z

dataplane/globalbase.cpp

+		else if (type == common::idp::updateGlobalBase::requestType::update_host_config)
+		{
+			result = update_host_config(std::get<common::idp::updateGlobalBase::update_host_config::request>(data));
+		}


This commit is about supporting some kind of ICMP response message.
Could you clarify these changes please?
I'm not following why we have this "update_host_config" sub-theme going on.

The icmp response should contain the IP of the current local device. It was decided that the IP of the local device will be obtained from the configuration.
A new section was introduced in the config (), which is passed to Dataplane

Then it should be done in a separate commit, otherwise it's hard to grasp the intentions, especially from the commit title.
I suggest to split the first commit into two -- one that will add that update mechanism with commit description equal to what you told me here, and the other that will actually do ICMP-related things

common/type.h

dataplane/type.h

ol-imorozko · 2025-02-10T13:41:31Z

Also we shouldn't have commits like "fix comments" etc.
If you fix something addressed to a certain commit, you should just change that commit accordingly and force-push branch.

ol-imorozko · 2025-02-13T15:01:59Z

dataplane/type.h

+		uint8_t zero_buf[16]{};
+		return !memcmp(bytes, zero_buf, std::size(bytes));


Still, we don't need to create temporary object/array for comparison. std::all_of is perfectly fine:

[[nodiscard]] bool is_default() const { return std::all_of(address.begin(), address.end(), [](uint8_t byte) { return byte == 0; }); }

ol-imorozko · 2025-02-13T15:04:01Z

dataplane/icmp.cpp

+namespace
+{
+const uint8_t TTL = 128;
+}


We don't need an anonymous namespace since we're using that only in this cpp file.
I suggest just constexpr uint8_t TTL = 128;

ol-imorozko · 2025-02-13T15:04:53Z

dataplane/icmp.cpp

+uint32_t create_icmp_package_time_exceeded(rte_mbuf* mbuf_target, const CreateTimeExceededPackagePayload& payload)
+{
+	if (mbuf_target == nullptr || payload.mbuf_source == nullptr)
+	{
+		return false;
+	}


return false when the function returns uint32_t?

ol-imorozko · 2025-02-13T15:05:52Z

dataplane/icmp.cpp

+				uint8_t addr[16];
+				rte_memcpy(addr, ip_header->dst_addr, 16);
+				rte_memcpy(ip_header->dst_addr, ip_header->src_addr, 16);
+				rte_memcpy(ip_header->src_addr, addr, 16);


I believe we can use RTE_SWAP for this

ol-imorozko · 2025-02-13T15:07:15Z

dataplane/icmp.cpp

+		// copy network header and next 64bits of source package (RFC 792)
+		rte_ipv4_hdr* ip_header = rte_pktmbuf_mtod_offset(mbuf_target, rte_ipv4_hdr*, metadata->network_headerOffset);
+		rte_memcpy(rte_pktmbuf_mtod_offset(mbuf_target, char*, metadata->transport_headerOffset + sizeof(rte_icmp_hdr)),
+		           rte_pktmbuf_mtod_offset(payload.mbuf_source, char*, YADECAP_METADATA(payload.mbuf_source)->network_headerOffset),


why YADECAP_METADATA(payload.mbuf_source) instead of just metadata?

ol-imorozko · 2025-02-13T15:07:26Z

dataplane/icmp.cpp

+		rte_ipv6_hdr* ip_header = rte_pktmbuf_mtod_offset(mbuf_target, rte_ipv6_hdr*, metadata->network_headerOffset);
+
+		rte_memcpy(rte_pktmbuf_mtod_offset(mbuf_target, char*, metadata->transport_headerOffset + sizeof(rte_icmp_hdr)),
+		           rte_pktmbuf_mtod_offset(payload.mbuf_source, char*, YADECAP_METADATA(payload.mbuf_source)->network_headerOffset),


ol-imorozko · 2025-02-13T15:09:27Z

Overall LGTM apart from really tiny issues

ol-imorozko · 2025-02-13T15:10:39Z

Add support ICMP Time Exceeded Message is grammatically incorrect. I suggest to just use Support ICMP Time Exceeded Message. Also it would be really helpful if you add commit description about why we need this. You can just copy issue description and have smth like:
Support ICMP Time Exceeded Message

YANET doesn't support ICMP response, when receiving packet with TTL = 1.
By RFC792 YANET should send ICMP Time Exceeded Message,
when packet cannot be forwarded or balanced.

Also do this, please. Not just in pr description, but in an actual commit message too

darkgrey1554 requested a review from GeorgyKirichenko as a code owner February 7, 2025 10:41

darkgrey1554 linked an issue Feb 7, 2025 that may be closed by this pull request

ICMP time exceeded #57

Open

darkgrey1554 commented Feb 7, 2025

View reviewed changes

dataplane/worker.cpp Outdated Show resolved Hide resolved

ezhk reviewed Feb 7, 2025

View reviewed changes

darkgrey1554 commented Feb 8, 2025

View reviewed changes

dataplane/icmp.cpp Outdated Show resolved Hide resolved

darkgrey1554 commented Feb 8, 2025

View reviewed changes

dataplane/worker.cpp Show resolved Hide resolved

darkgrey1554 force-pushed the icmp_ttl_exceeded_answer branch 5 times, most recently from e3986bf to 572877c Compare February 10, 2025 11:23

ol-imorozko requested changes Feb 10, 2025

View reviewed changes

darkgrey1554 changed the title ~~Add support ICMP Time Exceeded Message #issues57~~ Support ICMP Time Exceeded Message #issues57 Feb 11, 2025

darkgrey1554 force-pushed the icmp_ttl_exceeded_answer branch from 572877c to 8c028c0 Compare February 11, 2025 08:06

Add support ICMP Time Exceeded Message

2e7a727

darkgrey1554 force-pushed the icmp_ttl_exceeded_answer branch 5 times, most recently from ee2c91c to fb65b5f Compare February 11, 2025 08:54

Add payload to ICMP package

d04e3dc

darkgrey1554 force-pushed the icmp_ttl_exceeded_answer branch from fb65b5f to d04e3dc Compare February 11, 2025 09:03

darkgrey1554 requested a review from ol-imorozko February 13, 2025 12:02

ol-imorozko requested changes Feb 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support ICMP Time Exceeded Message #issues57 #274

Support ICMP Time Exceeded Message #issues57 #274

darkgrey1554 commented Feb 7, 2025

ol-imorozko commented Feb 10, 2025 •

edited

Loading

ol-imorozko Feb 10, 2025

darkgrey1554 Feb 11, 2025

ol-imorozko Feb 13, 2025

ol-imorozko commented Feb 10, 2025

ol-imorozko Feb 13, 2025

ol-imorozko Feb 13, 2025

ol-imorozko Feb 13, 2025

ol-imorozko Feb 13, 2025

ol-imorozko Feb 13, 2025

ol-imorozko Feb 13, 2025

ol-imorozko commented Feb 13, 2025

ol-imorozko commented Feb 13, 2025

		uint8_t zero_buf[16]{};
		return !memcmp(bytes, zero_buf, std::size(bytes));

Support ICMP Time Exceeded Message #issues57 #274

Are you sure you want to change the base?

Support ICMP Time Exceeded Message #issues57 #274

Conversation

darkgrey1554 commented Feb 7, 2025

ol-imorozko commented Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ol-imorozko commented Feb 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ol-imorozko commented Feb 13, 2025

ol-imorozko commented Feb 13, 2025

ol-imorozko commented Feb 10, 2025 •

edited

Loading