Skip to content

afpd: improved Spotlight RPC logic and bugfixes#2900

Merged
rdmark merged 12 commits intomainfrom
rdmark-spotlight-improvements
Apr 19, 2026
Merged

afpd: improved Spotlight RPC logic and bugfixes#2900
rdmark merged 12 commits intomainfrom
rdmark-spotlight-improvements

Conversation

@rdmark
Copy link
Copy Markdown
Member

@rdmark rdmark commented Apr 19, 2026

a range of improvements and bugfixes that should align closer to macOS Spotlight RPC behavior, based on analysis of Samba RPC logic

  • call stat() before add_filemeta() in tracker_cursor_cb
  • return Spotlight status 35 for all pre-DONE states in add_results()
  • handle fetchAllAttributes:forOIDArray:context: RPC for Spotlight
  • handle kMDItemContentModificationDate alias in Spotlight add_filemeta()
  • handle kMDItemContentCreationDate in Spotlight add_filemeta()
  • enrich fetchPropertiesForContext reply dict for Spotlight
  • Spotlight NFC to NFD path normalization in add_filemeta()
  • improve Spotlight logging for query tracing
  • guard against NULL query_results in tracker_cursor_cb
  • afpd: safer buffer length checks in spotlight
  • declare dalloc macros without final semicolon

@augmentcode
Copy link
Copy Markdown

augmentcode bot commented Apr 19, 2026

🤖 Augment PR Summary

Summary: This PR improves AFP Spotlight RPC compatibility with macOS, aligning responses and state handling more closely to observed client behavior.

Changes:

  • Normalize returned Spotlight path/name strings to NFD and add support for additional date attributes (creation/modification aliases)
  • Adjust query-result polling status logic to return status 35 for all pre-DONE states
  • Harden result processing by calling stat() before metadata extraction and improving query/result tracing logs
  • Handle fetchAllAttributes:forOIDArray:context: using the existing attributes RPC handler
  • Enrich fetchPropertiesForContext reply with additional scopes/UUID-related keys
  • Add Meson feature check for struct stat::st_birthtimespec

🤖 Was this summary useful? React with 👍 or 👎

Copy link
Copy Markdown

@augmentcode augmentcode bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review completed. 2 suggestions posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

Comment thread etc/afpd/spotlight.c Outdated
Comment thread etc/afpd/spotlight.c
@rdmark rdmark force-pushed the rdmark-spotlight-improvements branch from 174da71 to f466255 Compare April 19, 2026 06:09
@rdmark rdmark requested a review from andylemin April 19, 2026 06:20
@rdmark
Copy link
Copy Markdown
Member Author

rdmark commented Apr 19, 2026

@andylemin any idea why we got such a funky looking flamegraph?

@rdmark rdmark force-pushed the rdmark-spotlight-improvements branch from f466255 to 04ddadb Compare April 19, 2026 13:58
rdmark added 12 commits April 19, 2026 16:00
The struct stat sb was declared but never populated, causing add_filemeta()
to read garbage data for file size, owner UIDs/GIDs, and modification time
in every Spotlight query result. Add a stat() call before access(), and
skip the result on failure rather than propagating stale metadata.
…ts()

Previously only SLQ_STATE_RUNNING returned status 35 ("results pending");
SLQ_STATE_RESULTS and SLQ_STATE_FULL fell through to status 0, which tells
the client the query is finished even while the result buffer is still
filling. This could cause the client to stop polling prematurely.

The correct condition, is state < SLQ_STATE_DONE: all of RUNNING, RESULTS, and
FULL return 0x23 (35); DONE and beyond return 0.
Samba's mdssvc maps this selector to the same handler as
fetchAttributes:forOIDArray:context:. Add it to the dispatch chain
to avoid logging an "unknown Spotlight RPC" error when macOS clients
send this variant.
…lemeta()

kMDItemContentModificationDate is an alias for kMDItemFSContentChangeDate,
both mapping to st_mtim. Samba's mdssvc handles both spellings; without
this, clients requesting the former receive nil instead of the mtime.
Add birth time support for the kMDItemContentCreationDate Spotlight
attribute, using st_birthtimespec where available (macOS, FreeBSD).
On platforms without birth time in struct stat the attribute returns
nil, matching the prior behaviour for unknown attributes.

Adds a meson.build detection for HAVE_STRUCT_STAT_ST_BIRTHTIMESPEC.
Add keys observed in macOS Spotlight server responses (via Samba traces):

- kMDSStoreMetaScopes: add kMDQueryScopeAllIndexed and
  kMDQueryScopeComputerIndexed alongside the existing kMDQueryScopeComputer
- kMDSVolumeUUID: echo the volume UUID under the second key name
- kMDSStoreIsBackup: reflect actual volume configuration (true when
  "time machine = yes", false otherwise) rather than hardcoding a value
- kMDSStoreSupportsVolFS: set true, as observed from macOS
macOS expects filenames and paths in NFD (decomposed) Unicode form.
Server-side paths from Tracker are NFC (composed). Without conversion,
Spotlight results with non-ASCII characters in their names may not
match what the macOS client expects.

Convert the path to NFD before returning kMDItemDisplayName,
kMDItemFSName, _kMDItemFileName and kMDItemPath using the existing
charset_decompose(CH_UTF8) facility. Falls back to the original path
if the conversion buffer cannot be allocated or conversion fails.
- tracker_cursor_cb: log reason for each skipped result (stat failed,
  access denied, no CNID, CNID not in client filter) and log path+CNID
  for each accepted result; downgrade missing CNID from log_error to
  log_debug as it is expected when files are moved or deleted after
  Tracker indexed them
- add_results: log result count and status code when dispatching a
  batch to the client
- sl_rpc_fetchQueryResultsForContext: log ctx1/ctx2 at entry and emit
  an error log when no matching query is found for the context
add_results() transfers ownership of query_results to the client array
and sets slq->query_results = NULL before calling create_result_handle().
If create_result_handle() fails the state becomes SLQ_STATE_ERROR but
query_results stays NULL; a subsequent async cursor callback would then
dereference it in the initial LOG and tracker_to_unix_path(), crashing
under low-memory or other error paths.

Add an early NULL check at the top of tracker_cursor_cb() so the
callback returns cleanly in this case.
in this codebase we use the no final semicolon pattern for macros elsewhere,
so let's do the same in dalloc to avoid empty statements in usage sites
@rdmark rdmark force-pushed the rdmark-spotlight-improvements branch from 04ddadb to 387c0cb Compare April 19, 2026 14:00
@sonarqubecloud
Copy link
Copy Markdown

@github-actions
Copy link
Copy Markdown
Contributor

🔥 Spectest (AFP 3.4) - Flamegraph (AFP_ASSERT active)

Commit: 387c0cb00513accdf9a1090389124963b0f27f97
Profiling: On-CPU sampling @ 1009 Hz (prime), DWARF call-graph, x86_64
Build: debugoptimized (-O2 -g -fno-omit-frame-pointer)
Total Runtime: 66s, Netatalk Code-time: 4.0%,
Stacks: 1529, SVG size: 932K

🔥 Open interactive Flamegraph (SVG)

Flamegraph preview

📥 Download from artifacts →

🔝 Top 10 leaf functions
Function Samples
finish_task_switch.isra.0 525272400
do_syscall_64 285431040
_raw_spin_unlock_irqrestore 201189240
__cp_end 104063400
srso_alias_safe_ret 57482640
__syscall_cp_c 29732400
kmem_cache_alloc_noprof 21803760
dircache_process_deferred_chain 19821600
__x64_sys_close 19821600
find_get_block_common 18830520

@rdmark rdmark merged commit 0783ae9 into main Apr 19, 2026
55 checks passed
@rdmark rdmark deleted the rdmark-spotlight-improvements branch April 19, 2026 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants