On Sun, Feb 16, 2025 at 10:24:50PM GMT, Stanislav Brabec wrote:
> The new netlink based IP address lokup implements a new issue file escape \i.
> This lookup is based on netlink messages only. It evaluates all addresses
> reported in the netlink messages and it attempts to print only the best of them.
> As it is not possible to discriminate the "best" IP address, it reports all
> reasonably usable addresses.
I believe your code is a good start.
> Migrate \4 and \6 from getifaddrs() to netlink as well, and then completely drop
> getifaddrs() based code.
Yes, but it would be nice to make the code more readable and extendable :-)
My suggestion:
- Move the code to lib/netlink-ifaces.c.
- Add #ifdef TEST_PROGRAM with test main() and test_netlink to
lib/Makemodule.am (see for example lib/linux_version.c).
- Use include/debug.h and UL_NETLINK_DEBUG= environment variable (see
lib/loopdev.c, LOOPDEV_DEBUG_* and loopdev_init_debug()).
- Use a prefix for public functions (e.g., ul_iface_*).
- Create a set of small functions for basic operations:
- Add and remove interface (e.g., add_interface()).
- Add and remove address.
- Get the best address.
I also suggest using include/list.h for the lists, but you might find
it challenging at first. Ideally, define next() functions for while()
loops (e.g., ul_iface_next_interface() and ul_iface_next_addr()). See
for example libfdisk/src/table.c:fdisk_table_next_partition().
However, I'll understand if you think it's overkill.
The include/list.h provides a sort function, so it would be possible
to sort the lists by "quality", making the first address in the list
the best.
> It could be useful to implement:
> - configurable limit for the maximum number of interfaces that could be
> reported.
I believe the limit could be quite large by default (512?) since it's
just one malloc() for each address.
> - the interface regexp match
Yes, just another small function ul_iface_match_interface(), and if it
will be sorted than you get the best one :-)
\4{~eth[0-9]} or \4{match="~eth[0-9]"} if you want to support more
options in the { }.
- agetty already supports \foo{ } arguments, see get_escape_argument()
- lib/strutils.c:ul_optstr_next() can parse options list {a="aaa",b="bbb"}
> - an interface template for fine tuning of the output
You can introduce printf()-like \4{format="%iface:%addr} (like git-log)
> - better control over the output: e. g. disable printing of IPv4 or IPv6
> addresses, disable printing of temporary addresses or link-local addresses
> - cross-protocol checks: e. g. disable reporting of link-local IPv6 address for
> an IPv4-only interface
:-)
> The new network comparison code could potentially render obsolete string based
> issue comparison (implemented in 6522d88). There are ifaces_list_change_4 and
> ifaces_list_change_6 variables ready for this purpose.
Hmm... the global variables used deeply within the netlink message
parser do not look very elegant. Maybe introduce a separate struct,
ul_ifaces_status, and use it as a global variable in agetty.c,
then call process_netlink_msg() with this struct as an argument.
> +struct ip_quality_item {
> + enum ip_quality_item_value quality;
> + int len;
> + __u32 ifa_valid; /* IP addres lifetime */
uint32_t if possible ;-)
> + struct ip_quality_item *next;
> + char addr[];
> +};
> +struct iface_quality_item {
> + __u32 ifa_index;
> + struct ip_quality_item *ip_quality_list_4;
> + struct ip_quality_item *ip_quality_list_6;
> + struct iface_quality_item *next;
> +};
> +static struct iface_quality_item *ifaces_list = NULL;
> +bool ifaces_list_change_4;
> +bool ifaces_list_change_6;
> +bool ifaces_skip_dump = false;
Do we need the _quality_ in the sctruct names? What about ul_iface and
ul_iface_addr ?
...
> + debug_net("+ allocating new interface\n");
> + ifaceq = malloc(sizeof(struct iface_quality_item));
if (!ifaceq)
return;
> + ifaceq->ifa_index = ifaddr->ifa_index;
> + ifaceq->ip_quality_list_4 = NULL;
> + ifaceq->ip_quality_list_6 = NULL;
> + ifaceq->next = NULL;
> + *ifaceq_prev_next = ifaceq;
process_netlink_msg_part() will be simpler and more readable if you
move the basic tasks to small functions like add_interface() :-)
> + } else {
> + /* Should never happen */
> + debug_net("- interface not found\n");
> + return;
> + }
> + }
> + if (family == AF_INET) {
> + ipq_prev_next = &(ifaceq->ip_quality_list_4);
> + ifaces_list_change = &ifaces_list_change_4;
> + }
> + if (family == AF_INET6) {
> + ipq_prev_next = &(ifaceq->ip_quality_list_6);
> + ifaces_list_change = &ifaces_list_change_6;
> + }
I guess static analyzers won't like this part; ipq_prev_next is
uninitialized if the family is something else.
Karel
--
Karel Zak <kzak@xxxxxxxxxx>
http://karelzak.blogspot.com
