Skip to content

Conversation

wosayttn
Copy link
Contributor

拉取/合并请求描述:(PR description)

[
Original implementation always return default netif to caller.

We should compare source IP address and netif IP address with netmask at the same time.

Tested on multi-ethernet cards on Nuvoton platform.
]

当前拉取/合并请求的状态 Intent for your PR

必须选择一项 Choose one (Mandatory):

  • 本拉取/合并请求是一个草稿版本 This PR is for a code-review and is intended to get feedback
  • 本拉取/合并请求是一个成熟版本 This PR is mature, and ready to be integrated into the repo

代码质量 Code Quality:

我在这个拉取/合并请求中已经考虑了 As part of this pull request, I've considered the following:

  • 已经仔细查看过代码改动的对比 Already check the difference between PR and old code
  • 代码风格正确,包括缩进空格,命名及其他风格 Style guide is adhered to, including spacing, naming and other styles
  • 没有垃圾代码,代码尽量精简,不包含#if 0代码,不包含已经被注释了的代码 All redundant code is removed and cleaned up
  • 所有变更均有原因及合理的,并且不会影响到其他软件组件代码或BSP All modifications are justified and not affect other components or BSP
  • 对难懂代码均提供对应的注释 I've commented appropriately where code is tricky
  • 代码是高质量的 Code in this PR is of high quality
  • 已经使用formatting等源码格式化工具确保格式符合RT-Thread代码规范 This PR complies with RT-Thread code specification

We should compare source IP address and netif IP address with netmask at the same time.
Original implementation always return default netif to caller.
@mysterywolf mysterywolf requested a review from BernardXiong May 15, 2023 04:00
if (src != NULL)
{
if (ip4_addr_cmp(src, netif_ip4_addr(netif)))
if (ip4_addr_netcmp(src, netif_ip4_addr(netif), netif_ip4_netmask(netif)))
Copy link
Contributor

Choose a reason for hiding this comment

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

在这一步的src具体是谁?根据之前的经验发现src其实就是来自某个netif的addr,所以必然会出现src==netif->addr。而在多网卡的情况下,如果存在多个子网相同的情况ip4_addr_netcmp将取决于链表中的靠前的netif网卡对象。

所以请教一下lwip_ip4_route_src的真实意图到底是什么?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi,

並非某個 netif 的 IP 位址,依我的測試,我將 src, netif.ip 打印出來如下:
(1) HTTPD response時,src 是 HTTP client 的 IP Address。
(2) PING out 時,src 是 Target IP Address。

image

HTTPD:
image

PING out:
image

這個函式應該實作 forwarded PKT 的 next hop 是該往那個 network interface。
依原有的實作,幾乎是 Not match!! 所以總是往 default interface 傳送,導致不同網段的路由,完全失效 !!

Copy link
Contributor

Choose a reason for hiding this comment

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

按我的理解是src匹配路由的规则就是相当于socket 的bind操作。

而如果没有socket的bind操作的话,其实只要在此函数的末尾不要让他返回default对象,而是返回NULl就行了。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree and tested.
已修改成 route_src search 不到就回傳NULL; 後面還是有一個 search dest ip address 的 ip routing。

netdev_ping should execute with specified networking interface, not default_interface first.
@wosayttn wosayttn changed the title Fix next-hop finding in lwip_ip4_route_src. [components][net][lwip][port] Fix next-hop finding in lwip_ip4_route_src. May 16, 2023
Return NULL if have no matching in lwip_ip4_route_src.

This reverts commit 8958093.
@mysterywolf mysterywolf requested a review from geniusgogo May 16, 2023 21:58
@Guozhanxin Guozhanxin added the +1 Agree +1 label May 16, 2023
@geniusgogo geniusgogo added the +2 Agree +2 label May 17, 2023
@Guozhanxin Guozhanxin merged commit a2b7a44 into RT-Thread:master May 17, 2023
@wosayttn
Copy link
Contributor Author

Thanks.

@wosayttn wosayttn deleted the fix_lwip_ip4_route_src branch May 17, 2023 02:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
+1 Agree +1 +2 Agree +2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants