[pull] main from systemd:main - #992
Merged
Merged
Conversation
Move the logic that sends goodbye messages, removes a registered DNS-SD service's RRs from all mDNS zones and frees the service out of bus_dnssd_method_unregister() into a new helper. While at it, add the nowadays customary parameter comments and ", ignoring" log message suffix to the moved code. No functional change. This is preparation for unregistering services when the D-Bus client that registered them disconnects: that path needs the full teardown as well, not just dnssd_registered_service_free(), which would leave the service's RRs published in the per-link mDNS zones.
This is useful to check if how fast the code is (painfully slow) and whether worlds indeed are selected with a flat distribution probability (they are). The code is slow because the hashing and because chaseat is exteremely inefficient :(
search_and_fopen called search_and_fopen_internal which made a copy of the search strv and then called search_and_open which made a copy too.
112cfb1 tried to improve handling of paths when root is specified. It said: > To avoid answering what a relative path means when called with a root > prefix path_strv_canonicalize is now path_strv_canonicalize_absolute > and only considers absolute paths. Fortunately all users of already call > path_strv_canonicalize with a list of absolute paths. Of course, other users were added later and for example now SYSTEMD_HOSTNAME_WORDLIST_PATH=build/hostname-wordlist/ ... doesn't work as expected. If root is not specified, there is really no reason to ignore relative paths. They work relative to CWD per the usual convention. It's up to the user to not use relative paths in a context where they should not be used. When root is specified, continue to ignore the relative paths, since there is no clear interpreation what they mean, but emit a debug message.
hostname_pick_word() used to seek around in the wordlist file and read individual lines with read_line()/read_stripped_line(). Instead, slurp the whole file into memory use offset arithmetic. The wordlists are small, so reading them entirely is faster and there is really no reason to do the complicated thing. The new code is 15% faster (the slow parts are the hashing and chaseat, they actually dominate over the reading of the data). The "raced with truncation" case is gone, since the buffer is a consistent snapshot of the file. The random offsets derived from the machine ID are unchanged, so the picked words stay the same.
The comment was wrong. The average word is about 6 characters, so in 64 iterations, the probability of never landing on a word boundary is ~(1 - 1/7)**64, about 0.01%.
…ered dns_scope_announce(scope, /* goodbye= */ true) is not scoped to a specific service: it walks the whole per-link mDNS zone and flags every established RR as goodbye (TTL 0). Unregistering a single DNS-SD service via the UnregisterService() D-Bus call hence withdrew the host's own A/AAAA records and every other published service on the link from remote caches, with nothing re-announcing them afterwards. Introduce dns_scope_send_goodbye(), which sends an unsolicited response for a caller-provided RR set, and let the DNS-SD unregistration path send goodbye only for the RRs of the service actually being removed, mirroring the flag logic of dns_scope_announce(). The goodbye answer is built per scope, and only includes records actually established in the respective zone (or under re-verification, in which case they are still served from it): a per-service goodbye must take care not to withdraw records that are still being probed for uniqueness, were withdrawn again after a conflict (in which case the name belongs to the neighbor), or were never published in the scope in the first place (e.g. on MulticastDNS=resolve links, which have an mDNS scope but do not publish DNS-SD services). This becomes more important with the next commit, which promotes unregistration from an explicit method call to an automatic consequence of the registering client disconnecting from the bus.
…ishes Since the D-Bus DNS-SD API was introduced in c303664 ("resolved: implement D-Bus API for DNS-SD", v236), RegisterService() has set up an sd_bus_track object so that a registered service is destroyed when the client that registered it disconnects from the bus — there's even a handler logging "Client of active request vanished, destroying DNS-SD service." However, the track object was never stored anywhere: it is declared as a _cleanup_(sd_bus_track_unrefp) local in bus_method_register_service() and hence destroyed again when the method handler returns, taking the tracking down with it. The vanish handler was thus dead code, and services registered via D-Bus silently persisted after their client disconnected — unowned, impossible to re-register (the identifier stays taken, failing with DNSSD_SERVICE_EXISTS), and advertised until systemd-resolved itself exited. Store the track object in DnssdRegisteredService, pinning it for the lifetime of the service, so that client tracking works as originally intended. Route the vanish path through the full unregistration logic (goodbye packets + removal from the per-link mDNS zones), since freeing the service alone would leave its RRs published in the zones. Note for fire-and-forget clients (e.g. one-shot busctl/gdbus calls): these were never reliable — the registration silently vanished on any resolved restart — and now behave as documented for the corresponding Avahi API as well: the bus connection must be kept open for as long as the service is to remain registered. Document that in the man page and NEWS, and point to *.dnssd files for registrations independent of a client's lifetime.
Verify that a DNS-SD service registered via the D-Bus RegisterService() call is bound to the lifetime of the client's bus connection: since busctl disconnects right after the call returns, the service must be unregistered again shortly after. Wait for resolved's vanish handler log message as proof that the client tracking triggered the unregistration, check that a subsequent UnregisterService() fails specifically because the service is not known anymore, and verify that the identifier can be registered again — previously it stayed taken by the vanished client's service forever, failing with DNSSD_SERVICE_EXISTS. A dummy link with mDNS enabled makes sure the per-scope goodbye/zone teardown path actually runs, and since resolved would be restarted automatically if it crashed in it, letting the D-Bus level checks pass vacuously, verify the InvocationID stayed the same throughout.
shellcheck 0.10+ reports SC2329 ("This function is never invoked") for
the per-testcase cleanup() functions in TEST-75-RESOLVED.sh, which are
only invoked indirectly via trap. The existing SC2317 markers don't
cover it, and inserting a new testcase shuffles which definitions the
heuristic fires on, making pre-existing findings show up as new in
differential CI runs. Extend the markers to SC2317,SC2329, matching
what testcase_dot_strict_per_link_verify() in this file and other test
units already do.
…anishes (#43578) Since the D-Bus DNS-SD API was introduced in c3036641f0ed (v236), `RegisterService()` has created an `sd_bus_track` object to destroy the registered service when the registering client disconnects from the bus. However, the track object is a `_cleanup_(sd_bus_track_unrefp)` local in `bus_method_register_service()` that is never stored anywhere, so it is destroyed again as soon as the method handler returns. The vanish handler (`dnssd_registered_service_on_bus_track()`) has thus been dead code for the past ~8 years. The observable effect: services registered via D-Bus silently persist after the client disconnects — unowned, advertised until resolved exits, and impossible to re-register since the identifier stays taken (`DNSSD_SERVICE_EXISTS`). At the same time, the registration doesn't survive a resolved restart, so the current behavior is neither reliably ephemeral nor reliably persistent. Noticed while reviewing a project that (unknowingly relying on this bug) registers a service with a one-shot `gdbus call` and expects it to persist. This PR: 1. splits the goodbye/zone-removal logic out of `bus_dnssd_method_unregister()` into a helper — the vanish path needs the full teardown, not just `dnssd_registered_service_free()`, which would leave the service's RRs published in the per-link mDNS zones; 2. stores the track object in `DnssdRegisteredService`, making client tracking work as originally intended; 3. documents the lifetime semantics in `org.freedesktop.resolve1(5)` (matching the equivalent Avahi D-Bus API, whose entry groups are likewise bound to the client connection) and points to `*.dnssd` files for registrations independent of a client's lifetime; 4. adds a TEST-75 case covering the connection-bound lifetime. Note this is a behavioral change for fire-and-forget clients (one-shot `busctl`/`gdbus` invocations). Arguably those were never working reliably (registrations silently vanished on any resolved restart, and stale entries blocked re-registration), but if this is deemed too risky, the alternative would be to drop the dead tracking code and codify the persist-until-exit behavior instead — I'd argue tracking as originally intended is the better contract. Verification: code compiles; the new integration test is included.
CPU architecture is determined by uname(), rather than the compile time macros like __i386__, __x86_64__, __aarch64__. Let's conditionalize with uname_architecture(). Follow-up for d518675.
manpages build fails on Arch with new lxml:
Traceback (most recent call last):
File "/work/src/tools/make-directive-index.py", line 169, in make_page
_extract_directives(directive_groups, formatting, page)
~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/work/src/tools/make-directive-index.py", line 18, in _extract_directives
t = xml_parse(page)
File "/work/src/tools/xml_helper.py", line 25, in xml_parse
doc = tree.parse(page, _parser)
File "src/lxml/etree.pyx", line 3717, in lxml.etree.parse
File "src/lxml/parser.pxi", line 2045, in lxml.etree._parseDocument
File "src/lxml/parser.pxi", line 2071, in lxml.etree._parseDocumentFromURL
File "src/lxml/parser.pxi", line 1962, in lxml.etree._parseDocFromFile
File "src/lxml/parser.pxi", line 1231, in lxml.etree._BaseParser._parseDocFromFile
File "src/lxml/parser.pxi", line 647, in lxml.etree._ParserContext._handleParseResultDoc
File "src/lxml/parser.pxi", line 765, in lxml.etree._handleParseResult
File "src/lxml/parser.pxi", line 689, in lxml.etree._raiseParseError
File "../src/man/hostname.xml", line 5
lxml.etree.XMLSyntaxError: Entity 'entities' not defined, line 5, column 11
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
File "/work/src/tools/make-directive-index.py", line 180, in <module>
_xml = make_page(_template_path, _xml_files)
File "/work/src/tools/make-directive-index.py", line 171, in make_page
raise ValueError('failed to process ' + page) from e
ValueError: failed to process ../src/man/hostname.xml
Relying on XMLParser() defaults leaves the external parameter entity
used to load custom-entities.ent unresolved. Request entity resolution explicitly.
Follow-up for 1a13e31
mdns_maintenance_query_complete() takes a reference on query->dnsservice_request and never drops it, so every successful mDNS maintenance query leaks one reference to the discovered service. The leak alone would be benign, but the service owns the one-shot event source that drives its maintenance schedule, and it only borrows a plain pointer to its service browser. Once the extra reference is outstanding, dns_service_browser_free() -> dns_remove_service() only drops the list's reference, so dns_service_free() never runs, the maintenance timer is never disabled, and the service outlives the browser it points at. The next time the timer fires, mdns_maintenance_query() dereferences the freed DnsServiceBrowser to reach the manager's event loop and to issue the query. Reaching it only takes a browse subscription whose record lives long enough for one maintenance query to be answered, followed by the subscription going away. Make the reference a _cleanup_ one, the way the sibling mdns_browse_service_query_complete() below already handles the reference it takes on the service browser.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )