Re: [Crash-utility] [PATCH v3] Enhance "net" command to display IPv6 address of network interface

On 2023/03/07 13:46, lijiang wrote: >> > + char str[INET6_ADDRSTRLEN + 1] = {0}; >> > + char buffer[INET6_ADDRSTRLEN + 4] = {0}; >> >> What are the +1 and +4 for? >> >> >> I noticed that the size of INET6_ADDRSTRLEN is 48 in kernel code as below: >> >> --- a/include/linux/inet.h >> +++ b/include/linux/inet.h >> +/* >> + * These mimic similar macros defined in user-space for inet_ntop(3). >> + * See /usr/include/netinet/in.h . >> + */ >> +#define INET_ADDRSTRLEN (16) >> +#define INET6_ADDRSTRLEN (48) >> >> >> And, the size of INET6_ADDRSTRLEN is 46 in my machine as below: >> >> # cat /usr/include/netinet/in.h |grep INET6_ADDRSTRLEN >> #define INET6_ADDRSTRLEN 46 I'm not sure why the kernel one is 48 (for a multiple of 8?), but the INET6_ADDRSTRLEN in glibc header should mean the longest result of inet_ntop() in glibc, and the latest glibc also has the same 46. So I don't think we need to consider the kernel one. >> >> >> The rfc2460 said that the IPv6 increases the IP address size from 32 bits to 128 bits. >> >> crash> struct in6_addr >> struct in6_addr { >> union { >> __u8 u6_addr8[16]; >> __be16 u6_addr16[8]; >> __be32 u6_addr32[4]; >> } in6_u; >> } >> SIZE: 16 >> >> Given that, maybe they should be defined like this? >> >> + char str[INET6_ADDRSTRLEN + 2] = {0}; >> + char buffer[INET6_ADDRSTRLEN + 2 + 2] = {0}; >> >> Not sure what's the best way for this case. >> >> Looking at the example in the man page of inet_pton(3), INET6_ADDRSTRLEN >> seems enough for the str and contains a null char. The buffer can have >> a comma and a space (", ") so +2 is enough? i.e. >> >> char str[INET6_ADDRSTRLEN] = {0}; >> char buffer[INET6_ADDRSTRLEN + 2] = {0}; >> >> >> > + uint len = 0; >> > + >> > + buf = *bufp; >> > + pos = strlen(buf); >> >> Ah nicely done :) >> >> > + >> > + readmem(devaddr + OFFSET(net_device_ip6_ptr), KVADDR, >> > + &ip6_ptr, sizeof(ulong), "ip6_ptr", FAULT_ON_ERROR); >> > + >> > + if (!ip6_ptr) >> > + return; >> > + >> > + if (VALID_MEMBER(inet6_ifaddr_if_list)) { >> > + struct list_data list_data, *ld; >> > + ulong cnt = 0, i; >> > + >> > + ld = &list_data; >> > + BZERO(ld, sizeof(struct list_data)); >> > + ld->flags |= LIST_ALLOCATE; >> > + ld->start = ip6_ptr + OFFSET(inet6_dev_addr_list); >> > + cnt = do_list(ld); >> > + >> > + for (i = 1; i < cnt; i++) { >> > + >> > + addr = ld->list_ptr[i] + OFFSET(inet6_ifaddr_addr); >> >> > + addr -= OFFSET(inet6_ifaddr_if_list); >> >> >> The above code is easy to understand, because it actually imitates the container_of(). >> >> But if you would like to have the same style as show_net_devices_v2() and show_net_devices_v3(), that's also fine to me. Yes, please. I've not seen the above style (do_list and minus OFFSET) in crash before. Although we may go with the above, I think generally a new style tends to cause an unexpected result. so I'd like to use the common style if possible.

Thanks, Kazu