Comment 106 for bug 1754671

> core: allow '~.' domain in ip4 and ip6 configs

commit bebafff7844228d018d17f0d11c5d6035dbf6a91,
lgtm, but the commit message no longer seams to match, does it?

> dns: use dns-priority to provide a preprocessed domain list to plugins

+ c_list_for_each_entry (ip_data, _ip_config_lst_head (self), ip_config_lst) {

the list head is accessed on every iteration. Could we pre-cache the value like:

    CList *ip_config_lst_head;

    ip_config_lst_head = _ip_config_lst_head (self);

    c_list_for_each_entry (ip_data, ip_config_lst_head, ip_config_lst) {

    (and below again)

+ int i, n, n_domains = 0;

these variables are used to iterate over a number of guint values. Like

  n = nm_ip_config_get_num_searches (ip_config);

where get_num_searches() returns a guint. Could we consistently use the correct type? (yes, I know that C might optimize signed for loops better, because it assumes that signed cannot overflow. But IMO consistent use of types is more important than what the compiler might do).

+ priority = nm_ip_config_get_dns_priority (ip_config);
+ nm_assert (priority != 0);

this invariant is not enforced by NMIPxConfig, so, you basically assert that all callers that create an NMIPxConfig properly initialize priority to a non-zero value. It asserts something that is two layers of code away. That is non-obvious. Not sure what to do about this. Ok, fine as is :)

add the wilcard domain to all non-VPN
        ^^^^^^^

+ parent_priority = GPOINTER_TO_INT (g_hash_table_lookup (ht, "."));
+ if (parent_priority < 0 && parent_priority < priority) {
+ *out_parent = ".";
+ *out_parent_priority = parent_priority;

is "." still right? Should this be "" to mean the wildcard domain?

could you add a
   nm_assert (!g_hash_table_contains (ht, domain));
to domain_is_shadowed() -- because that one is already checked before calling domain_is_shadowed().

         g_free (ip_data->domains.search);

I always like avoiding unnecessary copies. But in this case, "search" array will point inside strings owned by NMIPxConfig. That seems quite fragile to me. Should we not clone them? In practice it's not necessary, but it feels fragile. What can we do about that?

»···/* Free the array and return NULL if the only element was the ending NULL */
»···strv = (char **) g_ptr_array_free (domains, (domains->len == 1));

»···return _nm_utils_strv_cleanup (strv, FALSE, FALSE, TRUE);

skip_repeated=TRUE likely won't help much, because duplicated domains are probably not sorted after each other. Maybe sort them first? Maybe by cherry-picking "shared: add nm_utils_strv_sort() helper" from https://github.com/NetworkManager/NetworkManager/pull/91