[systemd-devel] [PATCH] resolved: re-add support for getting local domain from DHCP
Zbigniew Jędrzejewski-Szmek
zbyszek at in.waw.pl
Sat Aug 2 21:51:13 PDT 2014
On Tue, Jul 29, 2014 at 04:54:24PM -0700, Michael Marineau wrote:
> On Tue, Jul 29, 2014 at 3:37 PM, Zbigniew Jędrzejewski-Szmek
> <zbyszek at in.waw.pl> wrote:
> > On Tue, Jul 29, 2014 at 02:48:18PM -0700, Michael Marineau wrote:
> >> When the code for generating resolv.conf was moved from networkd to
> >> resolved the DHCP domain name code was dropped.
> >> ---
> >>
> >> This is a resend, rebased since some recent changes changed how this
> >> patch needed to be implemented.
> >>
> >> src/network/networkd-link.c | 13 +++++++++++++
> >> src/network/sd-network.c | 24 ++++++++++++++++++++++++
> >> src/resolve/resolved-link.c | 20 ++++++++++++++++++++
> >> src/resolve/resolved-link.h | 2 ++
> >> src/resolve/resolved-manager.c | 10 +++++++++-
> >> src/systemd/sd-network.h | 3 +++
> >> 6 files changed, 71 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c
> >> index 3b8b7ed..827c428 100644
> >> --- a/src/network/networkd-link.c
> >> +++ b/src/network/networkd-link.c
> >> @@ -2451,6 +2451,19 @@ int link_save(Link *link) {
> >> (address + 1 ? " " : ""));
> >>
> >> fputs("\n", f);
> >> +
> >> + fprintf(f, "DOMAINNAME=");
> >> +
> >> + if (link->network->dhcp_domainname &&
> >> + link->dhcp_lease) {
> >> + const char *domainname;
> >> +
> >> + r = sd_dhcp_lease_get_domainname(link->dhcp_lease, &domainname);
> >> + if (r >= 0)
> >> + fputs(domainname, f);
> >> + }
> >> +
> >> + fputs("\n", f);
> > Is it really necessary to write anything if the name is not available?
> > Other parts of this function don't write anyting in similar cases.
>
> I was just matching the above lines which may write DNS= or NTP= with
> blank values. I don't think it matters either way. Omitting
> DOMAINNAME= if it is blank certainly looks a little cleaner since the
> writes get squashed into a single fprintf. Will update.
You're right, I misread the surrounding code. It would probably be nice
to not write anything in thos cases but it should be a separate patch anyway,
that does all the places at once.
> >> if (link->dhcp_lease) {
> >> diff --git a/src/network/sd-network.c b/src/network/sd-network.c
> >> index bfb8321..a427a27 100644
> >> --- a/src/network/sd-network.c
> >> +++ b/src/network/sd-network.c
> >> @@ -175,6 +175,30 @@ _public_ int sd_network_get_ntp(int ifindex, char ***ret) {
> >> return network_get_strv("NTP", ifindex, ret);
> >> }
> >>
> >> +_public_ int sd_network_get_domainname(int ifindex, char **domainname) {
> >> + _cleanup_free_ char *s = NULL, *p = NULL;
> >> + int r;
> >> +
> >> + assert_return(ifindex > 0, -EINVAL);
> >> + assert_return(domainname, -EINVAL);
> >> +
> >> + if (asprintf(&p, "/run/systemd/netif/links/%d", ifindex) < 0)
> >> + return -ENOMEM;
> > Not terribly important, but please spell that as:
> >
> > char p[sizeof("/run/systemd/netif/links/") + DECIMAL_STRING_MAX(int)];
> > snprintf(p, sizeof(p), "/run/systemd/netif/links/%d", ifindex);
>
> This was copied verbatim from similar functions in this file, should I
> update the style of the others to match your suggestion? Why the
> preference of manually calculating a buffer length than using
> asprintf?
You're right, in this case asprintf is totally appropriate.
> >> + r = parse_env_file(p, NEWLINE, "DOMAINNAME", &s, NULL);
> >> + if (r == -ENOENT)
> >> + return -ENODATA;
> >> + else if (r < 0)
> >> + return r;
> >> + else if (!s)
> >> + return -EIO;
> >> +
> >> + *domainname = s;
> >> + s = NULL;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> static inline int MONITOR_TO_FD(sd_network_monitor *m) {
> >> return (int) (unsigned long) m - 1;
> >> }
> >> diff --git a/src/resolve/resolved-link.c b/src/resolve/resolved-link.c
> >> index 6ac7c5b..f6b7f6a 100644
> >> --- a/src/resolve/resolved-link.c
> >> +++ b/src/resolve/resolved-link.c
> >> @@ -77,6 +77,7 @@ Link *link_free(Link *l) {
> >> while (l->dns_servers)
> >> dns_server_free(l->dns_servers);
> >>
> >> + free(l->domainname);
> >> free(l);
> >> return NULL;
> >> }
> >> @@ -191,10 +192,29 @@ clear:
> >> return r;
> >> }
> >>
> >> +static int link_update_domainname(Link *l) {
> >> + char *domainname = NULL;
> >> + int r;
> >> +
> >> + assert(l);
> >> +
> >> + free(l->domainname);
> >> + l->domainname = NULL;
> >> +
> >> + r = sd_network_get_domainname(l->ifindex, &domainname);
> >> + if (r < 0)
> >> + return r;
> >> +
> >> + l->domainname = domainname;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> int link_update_monitor(Link *l) {
> >> assert(l);
> >>
> >> link_update_dns_servers(l);
> >> + link_update_domainname(l);
> >> link_allocate_scopes(l);
> >> link_add_rrs(l);
> >>
> >> diff --git a/src/resolve/resolved-link.h b/src/resolve/resolved-link.h
> >> index f58bd54..9730aec 100644
> >> --- a/src/resolve/resolved-link.h
> >> +++ b/src/resolve/resolved-link.h
> >> @@ -68,6 +68,8 @@ struct Link {
> >>
> >> RateLimit mdns_ratelimit;
> >> RateLimit llmnr_ratelimit;
> >> +
> >> + char *domainname;
> >> };
> >>
> >> int link_new(Manager *m, Link **ret, int ifindex);
> >> diff --git a/src/resolve/resolved-manager.c b/src/resolve/resolved-manager.c
> >> index a8715bd..253a97e 100644
> >> --- a/src/resolve/resolved-manager.c
> >> +++ b/src/resolve/resolved-manager.c
> >> @@ -522,6 +522,7 @@ int manager_write_resolv_conf(Manager *m) {
> >> const char *path = "/run/systemd/resolve/resolv.conf";
> >> _cleanup_free_ char *temp_path = NULL;
> >> _cleanup_fclose_ FILE *f = NULL;
> >> + const char *domainname = NULL;
> >> unsigned count = 0;
> >> DnsServer *s;
> >> Iterator i;
> >> @@ -542,13 +543,20 @@ int manager_write_resolv_conf(Manager *m) {
> >> "# resolv.conf(5) in a different way, replace the symlink by a\n"
> >> "# static file or a different symlink.\n\n", f);
> >>
> >> - HASHMAP_FOREACH(l, m->links, i)
> >> + HASHMAP_FOREACH(l, m->links, i) {
> >> LIST_FOREACH(servers, s, l->dns_servers)
> >> write_resolve_conf_server(s, f, &count);
> >>
> >> + if (!domainname && l->domainname)
> >> + domainname = l->domainname;
> >> + }
> >> +
> >> LIST_FOREACH(servers, s, m->dns_servers)
> >> write_resolve_conf_server(s, f, &count);
> >>
> >> + if (domainname)
> >> + fprintf(f, "domain %s\n", domainname);
> >> +
> >
> > How does this deal with empty domain names? Let's say the server is
> > misconfigure and returns "". Will this still be valid syntax? Also,
> > are the contents checked for special characters, etc, earlier?
>
> sd_network_get_domainname() returns NULL for empty strings following
> the behavior of parse_env_file() so simply checking for NULL elsewhere
> is sufficient.
OK. It seems that technically the patch is correct. But I'm less sure that
we actually want that. I mean your patch would promote one of the (potentially)
many domain names as special. It feels as if either any or none should be
used, ie. maybe 'search' is more appropriate... I'll let Tom or Lennart
comment on this.
Zbyszek
More information about the systemd-devel
mailing list