netutils/ping: improve ping and ping6 for get_host error handling#3404
netutils/ping: improve ping and ping6 for get_host error handling#3404masc2008 wants to merge 1 commit intoapache:masterfrom
Conversation
simbit18
requested review from
Donny9,
GUIDINGLI,
acassis,
anchao,
jerpelea,
linguini1,
michallenc,
raiden00pl and
xiaoxiang781216
system/ping6/ping6.c
Outdated
| ****************************************************************************/ | ||
|
|
||
| static void ping6_result(FAR const struct ping6_result_s *result) | ||
| void ping6_result(FAR const struct ping_result_s *result) |
There was a problem hiding this comment.
why change to public function
| * Private Types | ||
| ****************************************************************************/ | ||
|
|
||
| struct ping6_priv_s |
There was a problem hiding this comment.
why move internal strut to header file
|
|
||
| struct ping6_result_s; | ||
|
|
||
| struct ping6_info_s |
There was a problem hiding this comment.
please move the reorg change to new pr
There was a problem hiding this comment.
The purpose is to let ping_result_s and ping6_result_s share same data structure. since in the callback same pointer was transferred, they will be used in different callback. same reason to ping_info_s and ping6_info_s.
if they have have private structure, and changed privately, then it will have very high risk, not easy to maintain them. considering this, should let them share same.
There was a problem hiding this comment.
The key change is here: others are for this:
There was a problem hiding this comment.
ping and ping6 is totally seperated in the old implementation, so I would suggest that:
- Keep the origin design, and change the above code to posix_spawn
- Implement ipv4/ipv6 ping in one file
either is better than the current change which couple ping/ping6 in adhoc way.
There was a problem hiding this comment.
I would say ipv4/ipv6 ping should be in one file, and that is correct way. if choose that direction: FreeBSD version of ping can match the requirement, so why do we need do such basic code again? what I did is some work with accept able cost and fully leverage the current file. if re-org based current ping code, it will a new folder with some repeated data structures.
There was a problem hiding this comment.
After further thinking, I still prefer my current direction.
First "posix_spawn" has two issues:
1. it still reports an extra "get host error" if just dns just replies one address
2. it will loop each endless if no dns replies if just do "posix_spawn" (This is a really bad problem).
Current ping/ping6 are quite well implemented, and just have a small bug. "Implement another ping" doesn't solve this bug in current ping tools.
It is allowed that several standalone tools share some common structures and functions. what I do is this such way, ping/ping6 are quite similar tools, and they should have some common things that can be leveraged by some public structures and functions. (considering that all referred functions are in "lib_ping_pub.c"). we still can configure the system to have either ping or ping6, they still goes same logic as before if just configured either one of them. If both ping and ping6 are configured it's a bug with those nuttx ping tools, we must have a solution to deal with such case.
Please consider my two cents. : )
There was a problem hiding this comment.
@ankohuu , sorry, I am missed your message. your commit looks good, this version should be able to accepted by community.
There was a problem hiding this comment.
@xiaoxiang781216 could you have a check with latest commit. my friend @ankohuu accepted your proposal and implement one solution. Not sure if this one can meet your expectation. Thanks!
github-actions
bot
added
Size: S
Area: System
and removed
Area: Networking
Size: M
labels
masc2008
force-pushed
the
master
branch
3 times, most recently
from
a275727 to
3e93bf7
Compare
When ping only receives an IPv6 DNS response, the current command reports "ping_gethostip" and exits even though ping6 can still handle the target. Keep the existing ping and ping6 netutils interfaces unchanged and add a one-way fallback in the ping command. If IPv4 name resolution fails, ping will spawn ping6 with the same arguments and let ping6 report the final result. Signed-off-by: Shunchao Hu <ankohuu@gmail.com>
masc2008
left a comment
masc2008
left a comment
There was a problem hiding this comment.
please check version and leave your coment.
| int status; | ||
| int i; | ||
|
|
||
| child_argv = malloc(sizeof(FAR char *) * (argc + 1)); |
There was a problem hiding this comment.
remove malloc, let's modify argv[0] directly
4 participants
dns look for both IPv4/v6 when do dns_query, so when do ping, it should try both ipv4 and ipv6 address before return "ping_gethostip" error.
Note: Please adhere to Contributing Guidelines.
Summary
dns lookup for both IPv4/v6 when do dns_query, so when do ping, it should try both ipv4 and ipv6 address before return "ping_gethostip" error.
Impact
The network packet losing IPv4 or IPv6 dns resolve response is quite common in network pressure test. It will return get host error without this change. It's miss-leading report to end user. It's also quite common practice for linux to use one ping support both ping and ping6.
Testing
attach two network packet that packet losing for either v4 or v6.
Error log attached as below:
ping -c 5 www.baidu.com
ERROR: ping_gethostip(www.baidu.com) failed
based the latest commit, do a full test (I let the dns query lose ipv4 dns query)
Details
ping www.baidu.com
spawn_execattrs: Setting policy=2 priority=100 for pid=10
nxtask_activate: ping pid=10,TCB=0x4023d0a0
dns_query_callback: INFO: DNS query retry 1/3
udp_callback: flags: 20
sendto_eventhandler: flags: 20
udp_send: UDP payload: 31 (59) bytes
ipv4_build_header: IPv4 Packet: ipid:51, length: 59
udp_send: Outgoing UDP packet length: 59
udp_callback: flags: 20
eth_input: IPv4 frame
udp_callback: flags: 2
udp_eventhandler: flags: 2
udp_eventhandler: UDP done
dns_recv_response: ID 15715
dns_recv_response: Query 128
dns_recv_response: Error 0
dns_recv_response: Num questions 1, answers 3, authrr 0, extrarr 0
dns_parse_name: Compressed answer
dns_recv_response: Answer: type=0005, class=0001, ttl=00015e, length=000f
dns_parse_name: Compressed answer
dns_recv_response: Answer: type=001c, class=0001, ttl=000042, length=0010
dns_recv_response: IPv6 address: 2408:871a:2100:1b23:0000:00ff:b07a:7ebc
nxtask_activate: ping6 pid=11,TCB=0x4023d540
icmpv6_setsockopt_internal: option: 1
PING6 2408:871a:2100:1b23::ff:b07a:7ebc: 56 bytes of data
neighbor_dumpipaddr: Not found: 2408:871a:2100:1b23:0000:00ff:b07a:7ebc
neighbor_dumpentry: Entry found: fc00:0000:0000:0000:0000:0000:0000:0001
neighbor_dump_address:
sendto_eventhandler: flags: 800000
sendto_eventhandler: Send ICMPv6 ECHO request
ipv6_build_header: IPv6 Payload length: 64
sendto_request: Outgoing ICMPv6 packet length: 104
sendto_eventhandler: Resuming
neighbor_dumpentry: Entry found: fc00:0000:0000:0000:0000:0000:0000:0001
neighbor_dump_address:
neighbor_ethernet_out: Outgoing IPv6 Packet length: 118 (64)
eth_input: IPv6 frame
icmpv6_poll_eventhandler: flags: 2
icmpv6_datahandler: Buffered 64 bytes
icmpv6_readahead: Received 64 bytes (of 104)
56 bytes from 2408:871a:2100:1b23::ff:b07a:7ebc icmp_seq=0 time=116.0 ms