Karel Zak wrote:
On Sun, Feb 16, 2025 at 10:24:50PM GMT, Stanislav Brabec wrote:
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_*).
Makes sense.
- 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.
It could be done. Just add and delete interface shares lot of code, so
it could be one function.
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.
Sounds good. I'll look at it.
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.
Not for agetty. 512 interfaces surely ovefloods all terminals screen and
maybe even scrollback memory.
And if you have 512 network interfaces (e. g. modem servers at DSL
providers), then you probably will not want to report all those
interfaces IP addresses. But yes, the code could simply support it.
- 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 { }.
Sounds like a good syntax.
- agetty already supports \foo{ } arguments, see get_escape_argument()
- lib/strutils.c:ul_optstr_next() can parse options list {a="aaa",b="bbb"}
I see.
- 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 ;-)
The kernel ifa_valid is declared as __u32. If uint32_t is always equal,
I'll use it.
Do we need the _quality_ in the sctruct names? What about ul_iface and
ul_iface_addr ?
process_netlink_msg_part() will be simpler and more readable if you
move the basic tasks to small functions like add_interface() :-)
Actually, if we introduce generic library functions, then other
consumers may want to do something else than quality evaluation.
I can imagine a generic structure with pointer to custom data. And the
function will have a callback that will perform the needed operation.
+ } 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.
Should never happen, as we use RTMGRP_IPV4_IFADDR or RTMGRP_IPV6_IFADDR
for netlink_groups. But yes, better safe than sorry.
--
Best Regards / S pozdravem,
Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o. e-mail: sbrabec@xxxxxxxx
Křižíkova 148/34 (Corso IIa) tel: +420 284 084 060
186 00 Praha 8-Karlín fax: +420 284 084 001
Czech Republic http://www.suse.cz/
PGP: 830B 40D5 9E05 35D8 5E27 6FA3 717C 209F A04F CD76
