(In reply to Beniamino Galvani from comment #38)
> (In reply to Thomas Haller from comment #37)
> What do you mean by "full normalization"?
>
> We can convert 'domain.' into 'domain' because it's equivalent, but
> 'domain..' or 'my..domain' are invalid and they should be dropped. I
> think this would be the best approach.
I mean, to handle "my..domain". Either normalize the double . away, or verify it and drop it as invalid. If you don't do either, then "~.." will end up being treated like "~." which is wrong.
> If we normalize '~.' to '~', once we strip the ~ prefix the domain
> would become empty, which is not desirable in my opinion.
Yes, after dropping the default domain becomes "". It's not that you currently use the domain "." as-is. You do:
> > > dns: use dns-priority to provide a preprocessed domain list to plugins
> >
> > _LOGD ("update-dns: updating plugin %s", plugin_name);
> > + rebuild_domain_lists (self);
> > if (!nm_dns_plugin_update (plugin,
> >
> > do we need to rebuild the list every time? We know when an update changes
> > something. Can we cache the result, generate it when needed, and clear it
> > when something changes?
>
> If nothing changes, the DNS configuration hash will be the same and
> update_dns() won't be called at all by nm_dns_manager_end_updates().
>
> Ok, nm_dns_manager_set_hostname() calls update_dns() without checking
> the hash, but do we need another caching mechanism different from the
> existing one just for this case?
Maybe the hashing mechanism is anyway ugly, and should be dropped (in a future commit). Instead of implementing a SHA-hashing of ~some~ parameters, implement a cmp() function. It's more efficient, and easier to get right.
(In reply to Beniamino Galvani from comment #38)
> (In reply to Thomas Haller from comment #37)
> What do you mean by "full normalization"?
>
> We can convert 'domain.' into 'domain' because it's equivalent, but
> 'domain..' or 'my..domain' are invalid and they should be dropped. I
> think this would be the best approach.
I mean, to handle "my..domain". Either normalize the double . away, or verify it and drop it as invalid. If you don't do either, then "~.." will end up being treated like "~." which is wrong.
> If we normalize '~.' to '~', once we strip the ~ prefix the domain
> would become empty, which is not desirable in my opinion.
Yes, after dropping the default domain becomes "". It's not that you currently use the domain "." as-is. You do:
nm_streq (domain, ".") ? NULL : domain)
that could also be:
nm_streq (domain, "") ? NULL : domain)
or
#define DEFAULT_DOMAIN ""
nm_streq (domain, DEFAULT_DOMAIN) ? NULL : domain)
(maybe a define is in order either way).
> > > dns: use dns-priority to provide a preprocessed domain list to plugins domain_ lists (self); plugin_ update (plugin, manager_ end_updates( ). manager_ set_hostname( ) calls update_dns() without checking
> >
> > _LOGD ("update-dns: updating plugin %s", plugin_name);
> > + rebuild_
> > if (!nm_dns_
> >
> > do we need to rebuild the list every time? We know when an update changes
> > something. Can we cache the result, generate it when needed, and clear it
> > when something changes?
>
> If nothing changes, the DNS configuration hash will be the same and
> update_dns() won't be called at all by nm_dns_
>
> Ok, nm_dns_
> the hash, but do we need another caching mechanism different from the
> existing one just for this case?
Maybe the hashing mechanism is anyway ugly, and should be dropped (in a future commit). Instead of implementing a SHA-hashing of ~some~ parameters, implement a cmp() function. It's more efficient, and easier to get right.
> > > dns: sd-resolved: honor dns-priority
> >
> > - NMIPConfig *ip_config = elem->data;
> > + NMDnsIPConfigData *data = elem->data;
> > + NMIPConfig *ip_config = data->ip_config;
> >
> > this seems wrong.
>
> Why? Looks fine to me.
you are right.