Re: [PATCH] agetty: Implement netlink based IP address lookup

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  <[email protected]>
 http://karelzak.blogspot.com