Samba: Add service discovery using mDNS - #4742
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds configurable SMB service discovery over mDNS/DNS-SD. The change updates configuration and documentation, adds the ChangesSamba network discovery
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant smbd_run as smbd run script
participant gdbus
participant resolve1 as org.freedesktop.resolve1
participant smbd
smbd_run->>gdbus: Register _smb._tcp on port 445
gdbus->>resolve1: RegisterService
smbd_run->>smbd: Start smbd
smbd-->>smbd_run: Exit
smbd_run->>gdbus: Unregister hostname-derived service
gdbus->>resolve1: UnregisterService
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@samba/rootfs/etc/s6-overlay/s6-rc.d/smbd/finish`:
- Around line 17-25: Update the mDNS lifecycle across the startup registration
and the finish cleanup: persist the object path returned by RegisterService,
then have the finish script unregister that saved path whenever it exists,
regardless of the current mdns configuration. Remove the hostname-derived path
construction and preserve the existing UnregisterService call against the stored
handle.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ef74c770-3b38-4f27-8d27-ab0ff383fd34
📒 Files selected for processing (7)
samba/CHANGELOG.mdsamba/DOCS.mdsamba/Dockerfilesamba/config.yamlsamba/rootfs/etc/s6-overlay/s6-rc.d/smbd/finishsamba/rootfs/etc/s6-overlay/s6-rc.d/smbd/runsamba/translations/en.yaml
agners
left a comment
agners
left a comment
There was a problem hiding this comment.
Thanks for your PR! This goes into the right direction.
| - amd64 | ||
| hassio_api: true | ||
| host_network: true | ||
| host_dbus: true |
There was a problem hiding this comment.
Doing this directly from the app works, but it doesn't seem a good approach. It exposes much more attack surface to the app then necessary.
Ideally we extend the app configuration schema to allow service announcements, and let Supervisor handle the interaction with the host OS. We can take the arguments of org.freedesktop.resolve1.Manager.RegisterService as inspiration for the config API. See also my comment in #4468 (comment).
There was a problem hiding this comment.
I have to admit what you are proposing is beyond my current capabilities. I would argue that both the ssh and matter_server apps have host_dbus: true, so I'm not sure why samba should be treated differently. If you're still not in favor of using D-Bus, I'll have to defer to someone more skilled to implement your preference.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
Other than the debate over |
2 participants
Added the capability to register smb service in mDNS for service discovery. This uses the
RegisterServicecall on theorg.freedesktop.resolve1service so the OS level mDNS announcersystemd-resolvedis used.network_discoveryconfiguration option for control of this feature.glibpackage which providesgdbuscommand needed.host_dbus: truetoconfig.yamlto allow D-Bus access.runscript, usegdbuscommand to register smb service on port 445 in mDNS.finishscript, usegdbuscommand to unregister smb service in mDNS.Summary by CodeRabbit
Summary by CodeRabbit
New Features
Documentation
Chores