Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/22984 )
Change subject: KUDU-1457 [1/n] add IPv6 support to net utils ...................................................................... Patch Set 9: (5 comments) http://gerrit.cloudera.org:8080/#/c/22984/8/src/kudu/util/net/net_util.cc File src/kudu/util/net/net_util.cc: http://gerrit.cloudera.org:8080/#/c/22984/8/src/kudu/util/net/net_util.cc@193 PS8, Line 193: constexpr > is this really constexpr? Good catch! I guess I relied on compiler to figure that out. Status constructor does allocate memory so it may not be safe after all. Changed to const. http://gerrit.cloudera.org:8080/#/c/22984/8/src/kudu/util/net/net_util.cc@432 PS8, Line 432: host_.find(host_, ':') > The code calls host_.find(host_, ':'). Did we mean to search for a colon ra That's a bug that creeped in when I made transition from strcount to find in PS7. Thanks for catching this. http://gerrit.cloudera.org:8080/#/c/22984/8/src/kudu/util/net/net_util.cc@450 PS8, Line 450: return IN6_IS_ADDR_LOOPBACK(&addr); > IN6_IS_ADDR_LOOPBACK(&addr) expects const in6_addr*, but we pass a uint128_ Input addr is 4 32-bit integers packed in order into 128 bit integer type. If you see the callers, addr is created from struct in6_addr member inside struct sockaddr_in6. To convert, it would require additional memcpy with swapped source and destination as opposite to what caller is doing. However, your point may be valid from the type checking readability standpoint. I would prefer to keep it as is unless I am missing something. http://gerrit.cloudera.org:8080/#/c/22984/8/src/kudu/util/net/net_util.cc@463 PS8, Line 463: str > Should we handle the case where inet_ntop() fails and str remains uninitial The two possible error conditions could be unsupported family or inadequate dst buffer size. Since the method is being used to get text form and may be used directly in callers, I guess it would make sense to initialise 'str' to empty string to avoid any unforeseen events. http://gerrit.cloudera.org:8080/#/c/22984/8/src/kudu/util/net/sockaddr.h File src/kudu/util/net/sockaddr.h: http://gerrit.cloudera.org:8080/#/c/22984/8/src/kudu/util/net/sockaddr.h@112 PS8, Line 112: // Set the IP port for this address. : // REQUIRES: is an IP address. : void set_port(in_port_t port); : : // Get the IP port for this address. : // REQUIRES: is an IP address. : uint16_t port() const; > Should both methods use the same type for consistency? Indeed! -- To view, visit http://gerrit.cloudera.org:8080/22984 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I22c773ffb2ff44b9cd765b546d6724ec5543586f Gerrit-Change-Number: 22984 Gerrit-PatchSet: 9 Gerrit-Owner: Ashwani Raina <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Comment-Date: Wed, 30 Jul 2025 12:56:45 +0000 Gerrit-HasComments: Yes
