psh: add ipv6 support (ifconfig/ping)#246
Conversation
Summary of ChangesHello @rmikielis, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request adds support for displaying IPv6 addresses in the ifconfig command. The implementation is mostly correct, but I've found a couple of areas for improvement. One is a minor issue regarding how unspecified addresses are checked, which can be made more robust and readable. The other is a more significant issue in the new IPv6 printing logic, which involves confusing buffer reuse and a bug in error handling. I've provided suggestions to refactor this part to make it cleaner and more correct.
psh/ifconfig/ifconfig.c
Outdated
| msg = in6addrBuff; | ||
| inet_ntop(AF_INET6, &ioctlInterface6Address[i].ifr_addr6.sin6_addr, in6addrBuff, INET6_ADDRSTRLEN); | ||
| if (strlen(in6addrBuff) == 2) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The check strlen(in6addrBuff) == 2 is an obscure and inefficient way to check for an unspecified (all-zero) IPv6 address. It relies on the string representation being "::" and performs a string conversion just for the check. It's more robust, readable, and efficient to use the IN6_IS_ADDR_UNSPECIFIED macro on the binary address directly before converting it to a string.
if (IN6_IS_ADDR_UNSPECIFIED(&ioctlInterface6Address[i].ifr_addr6.sin6_addr)) {
continue;
}
msg = in6addrBuff;
inet_ntop(AF_INET6, &ioctlInterface6Address[i].ifr_addr6.sin6_addr, in6addrBuff, INET6_ADDRSTRLEN);bb5e4bc to
515b6ae
Compare
515b6ae to
77ba2a6
Compare
77ba2a6 to
81d135c
Compare
81d135c to
fd20f27
Compare
fd20f27 to
e5b486c
Compare
e5b486c to
b21a1d3
Compare
d0e55bb to
683c76b
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces IPv6 support to the psh shell, specifically for the ifconfig and ping commands. The changes involve adding necessary header files, conditional compilation based on the PSH_IPV6_SUPPORT macro, and implementing IPv6 address printing and pinging functionalities. The code includes new functions for calculating IPv6 prefix lengths and handling IPv6 addresses in ifconfig, as well as modifications to the ping command to support IPv6 echo requests and replies. I have identified some areas where improvements can be made, particularly in error handling and code clarity.
683c76b to
3c42ebd
Compare
3c42ebd to
1c15d81
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces IPv6 support for the ifconfig and ping commands. The ifconfig command is updated to display IPv6 addresses, prefix lengths, and scopes. The ping command is significantly refactored to handle both IPv4 and IPv6, controlled by a new -6 flag. The changes are well-structured, but I've found a critical issue in the IPv6 ping implementation related to setting the hop limit, as well as a few minor logic issues and areas for code simplification. My review includes suggestions to fix these issues.
JIRA: RTOS-1160
1c15d81 to
bbb87ac
Compare
|
Consider adding yourself to the authors list. |
JIRA: RTOS-1160
bbb87ac to
469291c
Compare
JIRA: RTOS-1160
add psh/ifconfig feature to show ipv6 address assigned
add psh/ping support for ipv6 messages
Description
Dependency tree:
phoenix-rtos/libphoenix#447 (merged to master)
phoenix-rtos/libphoenix#450
Motivation and Context
Types of changes
How Has This Been Tested?
Checklist:
Special treatment