Hi Chris,
On Tue, Nov 3, 2015 at 4:11 AM, Chris Packham judge.packham@gmail.com wrote:
Hi Joe,
I've answered a few questions below. I'll address your comments more completely before sending another round next week (or I can sit on it for longer if you want me to give you some breathing room).
Feel free to send it when you're ready.
On Tue, Nov 3, 2015 at 9:43 AM, Joe Hershberger joe.hershberger@gmail.com wrote:
Hi Chris,
On Mon, Oct 12, 2015 at 2:43 AM, Chris Packham judge.packham@gmail.com wrote:
Adds basic support for IPv6. Neighbor discovery and ping6 are the only things supported at the moment.
Helped-by: Hanna Hawa hannah@marvell.com [endian & alignment fixes] Signed-off-by: Chris Packham judge.packham@gmail.com
Now we have something functional. With this you can do something like 'setenv ipaddr6 3ffe::2' and 'ping6 3ffe::1' should work.
I seem to have a problem that when you send a ping6 for a non-existent address that ends up stuck and the next non-ipv6 net operation tries to resolve it. I suspect this is because the pending neighbor discovery information isn't cleaned up properly, I need to look into that.
The environment variable prefixlength6 is a bit fiddly. No-one uses a netmask6 (it'd mean a lot of ffff:ffff:...) it's almost always done by including the prefix length in the address (e.g. ip addr add 2001:db8::1/64) I'm contemplating adopting that syntax and dropping prefixlength6.
This patch is bigger than I'd like it to be but I'm not sure how to split it up and keep the parts build able.
It is pretty huge. It's taken me a while to get through it.
Thanks for making the effort to review. I realise it's pretty huge and I'll put some effort into splitting it futher. One obvious thing that could be split out is the new environment variables.
Sounds good.
common/Kconfig | 15 ++ common/cmd_net.c | 28 ++++ include/env_callback.h | 9 ++ include/env_flags.h | 10 ++ include/net.h | 5 +- include/net6.h | 212 ++++++++++++++++++++++++++++ net/Kconfig | 5 + net/Makefile | 3 + net/ndisc.c | 269 +++++++++++++++++++++++++++++++++++ net/ndisc.h | 27 ++++ net/net.c | 36 ++++- net/net6.c | 375 +++++++++++++++++++++++++++++++++++++++++++++++++ net/ping6.c | 111 +++++++++++++++ 13 files changed, 1102 insertions(+), 3 deletions(-) create mode 100644 net/ndisc.c create mode 100644 net/ndisc.h create mode 100644 net/net6.c create mode 100644 net/ping6.c
diff --git a/common/Kconfig b/common/Kconfig index 2c42b8e..c72563d 100644 --- a/common/Kconfig +++ b/common/Kconfig @@ -389,6 +389,15 @@ config CMD_NET bootp - boot image via network using BOOTP/TFTP protocol tftpboot - boot image via network using TFTP protocol
+config CMD_NET6
bool "ipv6 commands"select NETselect NET6default nhelpIPv6 network commandstftpboot6 - boot image via network using TFTP protocolThis is added in the next patch, so should probably move there.
Will do.
config CMD_TFTPPUT bool "tftp put" help @@ -420,6 +429,12 @@ config CMD_PING help Send ICMP ECHO_REQUEST to network host
+config CMD_PING6
bool "ping6"depends on CMD_NET6helpSend ICMPv6 ECHO_REQUEST to network hostWhat makes ping inseparable from the core support?
Mainly testing, I can't test the core code without ping. But I can see that from a patch submission point of few splitting it out will make review easier.
One simple way to prove out the core without ping would maybe be to rebase and switch tftp with ping and make sure that tftp works without ping. Then you at least know you have enough, but you won't know if you left in more than you need.
config CMD_CDP bool "cdp" help
--8<---- snip ----8<--
diff --git a/include/env_callback.h b/include/env_callback.h index 90b95b5..9027f3f 100644 --- a/include/env_callback.h +++ b/include/env_callback.h @@ -60,6 +60,14 @@ #define NET_CALLBACKS #endif
+#ifdef CONFIG_NET6 +#define NET6_CALLBACKS \
"ip6addr:ip6addr," \"serverip6:serverip6," \"prefixlength6:prefixlength6,"I like the other nomenclature better as well (included in the address).
I've actually already implemented the code to include the prefixlength in the address and it makes things a lot more usable (the parsing code is a bit more complicated).
Sounds good.
+#else +#define NET6_CALLBACKS +#endif /*
- This list of callback bindings is static, but may be overridden by defining
- a new association in the ".callbacks" environment variable.
@@ -68,6 +76,7 @@ ENV_DOT_ESCAPE ENV_FLAGS_VAR ":flags," \ "baudrate:baudrate," \ NET_CALLBACKS \
NET6_CALLBACKS \ "loadaddr:loadaddr," \ SILENT_CALLBACK \ SPLASHIMAGE_CALLBACK \
--8<---- snip ----8<--
+void ip6_NDISC_Request(void)
Please don't use CamelCase looking stuff. All lower case is usually appropriate.
Will fix. The original Allied Telesis implementation was based on a fairly old verison and copied the non-standard net.c which was recently updated to eliminate camelcase.
Also I was wondering about "ip6_ndisc" vs "ndisc". The latter is sorter and kind of fits with "arp" for ipv4 but the difference is that unlike arp, neigbor discovery actually operates over ipv6.
I think it's fine to just use ndisc, since it is not going to be ambiguous, but up to you if you want to change it.
+{
if (!ip6_addr_in_subnet(&net_ip6, &net_nd_sol_packet_ip6,net_prefix_length)) {if (ip6_is_unspecified_addr(&net_gateway6)) {puts("## Warning: gatewayip6 is needed but not set\n");net_nd_rep_packet_ip6 = net_nd_sol_packet_ip6;Is this just assuming that since there is no gateway that the device might be on the local segment even though the address is not on our subnet?
Yeah I might need to add some conditions on the configured address not being the link local one.
It seems like if your ip is link local you don't want to use the gateway even if you have one configured.
} else {net_nd_rep_packet_ip6 = net_gateway6;}} else {net_nd_rep_packet_ip6 = net_nd_sol_packet_ip6;}ip6_send_ns(&net_nd_rep_packet_ip6);+}
--8<---- snip ----8<--
+int +ip6_addr_in_subnet(struct in6_addr *our_addr, struct in6_addr *neigh_addr,
__u32 plen)+{
__be32 *addr_dwords;__be32 *neigh_dwords;addr_dwords = our_addr->s6_addr32;neigh_dwords = neigh_addr->s6_addr32;while (plen > 32) {if (*addr_dwords++ != *neigh_dwords++)return 0;plen -= 32;}/* Check any remaining bits. */if (plen > 0) {/* parameters are in network byte order.Does this work on a LE host? */So is this still an outstanding question? Probably worth testing on a LE target, but it should work I believe.
I'm actually testing with x86 on QEMU so I think LE is all good. I'll remove the comment.
Great. Have you done any testing in sandbox? I'd really like to see unit tests go in as part of this series.
if ((*addr_dwords >> (32 - plen)) !=(*neigh_dwords >> (32 - plen))) {return 0;}}return 1;+}
--8<---- snip ----8<--