From a5e6c8498ca375bafa865d5e46fa95e9313871ad Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 4 Jan 2023 16:29:34 +0100 Subject: [PATCH 1/3] resolvectl: fix type of ifindex D-Bus field, and make sure to initialize to zero in all code paths --- src/resolve/resolvectl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/resolve/resolvectl.c b/src/resolve/resolvectl.c index 5889bd772f..4d2d9a1e17 100644 --- a/src/resolve/resolvectl.c +++ b/src/resolve/resolvectl.c @@ -1209,9 +1209,10 @@ static int reset_server_features(int argc, char **argv, void *userdata) { static int read_dns_server_one(sd_bus_message *m, bool with_ifindex, bool extended, char **ret) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; _cleanup_free_ char *pretty = NULL; - int ifindex, family, r, k; union in_addr_union a; const char *name = NULL; + int32_t ifindex = 0; + int family, r, k; uint16_t port = 0; assert(m); From 5707fb12978e6818afba379b803da307050f541b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 4 Jan 2023 16:32:51 +0100 Subject: [PATCH 2/3] resolved: add some line-breaks/comments Let's make this a bit more readable. --- src/resolve/resolvectl.c | 20 +++++++++++++++----- src/resolve/resolved-bus.c | 12 ++++++++++-- 2 files changed, 25 insertions(+), 7 deletions(-) diff --git a/src/resolve/resolvectl.c b/src/resolve/resolvectl.c index 4d2d9a1e17..d92ef1646f 100644 --- a/src/resolve/resolvectl.c +++ b/src/resolve/resolvectl.c @@ -1218,7 +1218,11 @@ static int read_dns_server_one(sd_bus_message *m, bool with_ifindex, bool extend assert(m); assert(ret); - r = sd_bus_message_enter_container(m, 'r', with_ifindex ? (extended ? "iiayqs" : "iiay") : (extended ? "iayqs" : "iay")); + r = sd_bus_message_enter_container( + m, + 'r', + with_ifindex ? (extended ? "iiayqs" : "iiay") : + (extended ? "iayqs" : "iay")); if (r <= 0) return r; @@ -1263,7 +1267,6 @@ static int read_dns_server_one(sd_bus_message *m, bool with_ifindex, bool extend return r; *ret = TAKE_PTR(pretty); - return 1; } @@ -1730,7 +1733,14 @@ static int status_ifindex(sd_bus *bus, int ifindex, const char *name, StatusMode return 0; } -static int map_global_dns_servers_internal(sd_bus *bus, const char *member, sd_bus_message *m, sd_bus_error *error, void *userdata, bool extended) { +static int map_global_dns_servers_internal( + sd_bus *bus, + const char *member, + sd_bus_message *m, + sd_bus_error *error, + void *userdata, + bool extended) { + char ***l = ASSERT_PTR(userdata); int r; @@ -1767,11 +1777,11 @@ static int map_global_dns_servers_internal(sd_bus *bus, const char *member, sd_b } static int map_global_dns_servers(sd_bus *bus, const char *member, sd_bus_message *m, sd_bus_error *error, void *userdata) { - return map_global_dns_servers_internal(bus, member, m, error, userdata, false); + return map_global_dns_servers_internal(bus, member, m, error, userdata, /* extended= */ false); } static int map_global_dns_servers_ex(sd_bus *bus, const char *member, sd_bus_message *m, sd_bus_error *error, void *userdata) { - return map_global_dns_servers_internal(bus, member, m, error, userdata, true); + return map_global_dns_servers_internal(bus, member, m, error, userdata, /* extended= */ true); } static int map_global_current_dns_server(sd_bus *bus, const char *member, sd_bus_message *m, sd_bus_error *error, void *userdata) { diff --git a/src/resolve/resolved-bus.c b/src/resolve/resolved-bus.c index 2e3941da39..c818e1d52f 100644 --- a/src/resolve/resolved-bus.c +++ b/src/resolve/resolved-bus.c @@ -1371,7 +1371,11 @@ static int bus_method_resolve_service(sd_bus_message *message, void *userdata, s return 1; } -int bus_dns_server_append(sd_bus_message *reply, DnsServer *s, bool with_ifindex, bool extended) { +int bus_dns_server_append( + sd_bus_message *reply, + DnsServer *s, + bool with_ifindex, /* include "ifindex" field */ + bool extended) { /* also include port number and server name */ int r; assert(reply); @@ -1390,7 +1394,11 @@ int bus_dns_server_append(sd_bus_message *reply, DnsServer *s, bool with_ifindex } } - r = sd_bus_message_open_container(reply, 'r', with_ifindex ? (extended ? "iiayqs" : "iiay") : (extended ? "iayqs" : "iay")); + r = sd_bus_message_open_container( + reply, + 'r', + with_ifindex ? (extended ? "iiayqs" : "iiay") : + (extended ? "iayqs" : "iay")); if (r < 0) return r; From 889a1b9f4e799b31f1be06db74708aa8beb70829 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 4 Jan 2023 16:36:15 +0100 Subject: [PATCH 3/3] resolvectl: don't filter loopback DNS server from global DNS server list "resolvectl status" shows per-link DNS servers separately from global ones. When querying the global list, it will contain both per-link and global servers however. Thus, to not show duplicate info we filter all entries that actually have a non-zero ifindex set (under the assumption that that's a per-link server). This doesn't work if people configured 127.0.0.1 as global server though, as we'll add ifindex 1 to it since 6e32414a66ff8dbcef233981a7066684d903ee9f unconditionally even for global servers. Let's address that by excluding entries with ifindex 1 from suppression. This is safe as resolved ignores loopback ifaces, hence never will have per-link servers on ifindex 1. Note that this splits up the "with_ifindex" parameter into a second parameter "only_global", since they semantically do two different things. One controls whether we shall expect/parse an ifindex dbus field. The other controls whether we shall filter all ifindex values set != 0. These are effectively always used in conjunction hence making them the same actually worked. However this is utterly confusing I think, which as I guess is resulting in the confusion around #25796 (which removes the whole check) Replaces: #25796 --- src/resolve/resolvectl.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/src/resolve/resolvectl.c b/src/resolve/resolvectl.c index d92ef1646f..682fe87ba9 100644 --- a/src/resolve/resolvectl.c +++ b/src/resolve/resolvectl.c @@ -1206,7 +1206,13 @@ static int reset_server_features(int argc, char **argv, void *userdata) { return 0; } -static int read_dns_server_one(sd_bus_message *m, bool with_ifindex, bool extended, char **ret) { +static int read_dns_server_one( + sd_bus_message *m, + bool with_ifindex, /* read "ifindex" reply that also carries an interface index */ + bool extended, /* read "extended" reply, i.e. with port number and server name */ + bool only_global, /* suppress entries with an (non-loopback) ifindex set (i.e. which are specific to some interface) */ + char **ret) { + _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; _cleanup_free_ char *pretty = NULL; union in_addr_union a; @@ -1256,8 +1262,8 @@ static int read_dns_server_one(sd_bus_message *m, bool with_ifindex, bool extend return 1; } - if (with_ifindex && ifindex != 0) { - /* only show the global ones here */ + if (only_global && ifindex > 0 && ifindex != LOOPBACK_IFINDEX) { + /* This one has an (non-loopback) ifindex set, and we were told to suppress those. Hence do so. */ *ret = NULL; return 1; } @@ -1285,7 +1291,7 @@ static int map_link_dns_servers_internal(sd_bus *bus, const char *member, sd_bus for (;;) { _cleanup_free_ char *pretty = NULL; - r = read_dns_server_one(m, false, extended, &pretty); + r = read_dns_server_one(m, /* with_ifindex= */ false, extended, /* only_global= */ false, &pretty); if (r < 0) return r; if (r == 0) @@ -1318,14 +1324,14 @@ static int map_link_current_dns_server(sd_bus *bus, const char *member, sd_bus_m assert(m); assert(userdata); - return read_dns_server_one(m, false, false, userdata); + return read_dns_server_one(m, /* with_ifindex= */ false, /* extended= */ false, /* only_global= */ false, userdata); } static int map_link_current_dns_server_ex(sd_bus *bus, const char *member, sd_bus_message *m, sd_bus_error *error, void *userdata) { assert(m); assert(userdata); - return read_dns_server_one(m, false, true, userdata); + return read_dns_server_one(m, /* with_ifindex= */ false, /* extended= */ true, /* only_global= */ false, userdata); } static int read_domain_one(sd_bus_message *m, bool with_ifindex, char **ret) { @@ -1755,7 +1761,7 @@ static int map_global_dns_servers_internal( for (;;) { _cleanup_free_ char *pretty = NULL; - r = read_dns_server_one(m, true, extended, &pretty); + r = read_dns_server_one(m, /* with_ifindex= */ true, extended, /* only_global= */ true, &pretty); if (r < 0) return r; if (r == 0) @@ -1785,17 +1791,11 @@ static int map_global_dns_servers_ex(sd_bus *bus, const char *member, sd_bus_mes } static int map_global_current_dns_server(sd_bus *bus, const char *member, sd_bus_message *m, sd_bus_error *error, void *userdata) { - assert(m); - assert(userdata); - - return read_dns_server_one(m, true, false, userdata); + return read_dns_server_one(m, /* with_ifindex= */ true, /* extended= */ false, /* only_global= */ true, userdata); } static int map_global_current_dns_server_ex(sd_bus *bus, const char *member, sd_bus_message *m, sd_bus_error *error, void *userdata) { - assert(m); - assert(userdata); - - return read_dns_server_one(m, true, true, userdata); + return read_dns_server_one(m, /* with_ifindex= */ true, /* extended= */ true, /* only_global= */ true, userdata); } static int map_global_domains(sd_bus *bus, const char *member, sd_bus_message *m, sd_bus_error *error, void *userdata) {