Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

lib/osutil: Only announce address of interfaces which are up (fixes #7458) #8422

Merged
merged 1 commit into from Jul 7, 2022
Merged

lib/osutil: Only announce address of interfaces which are up (fixes #7458) #8422

merged 1 commit into from Jul 7, 2022

Conversation

Zahrun
Copy link
Contributor

@Zahrun Zahrun commented Jul 7, 2022 •
edited

Purpose

Syncthing will only announce address of interfaces which are up (fixes #7458)

Testing

Syncthing compiles
Check that Syncthing is announcing only addresses of interfaces which are up

Documentation

Not sure if the change should be mentioned in the documentation

calmh
calmh reviewed Jul 7, 2022
View reviewed changes
lib/osutil/lan.go Outdated Show resolved Hide resolved
calmh
calmh reviewed Jul 7, 2022
View reviewed changes
lib/osutil/lan.go Outdated Show resolved Hide resolved
@calmh calmh changed the title lib/osutil: only announce address of interfaces which are up (fixes #7458) lib/osutil: Only announce address of interfaces which are up (fixes #7458) Jul 7, 2022
@calmh calmh merged commit 7cb8af9 into syncthing:main Jul 7, 2022
@acolomb
Copy link
Member

acolomb commented Jul 7, 2022

Will this still cause interfaces to be announced when they come up later? Is there a periodic check for new addresses? Sorry I didn't have a chance to look at the code yet.

@Zahrun
Copy link
Contributor Author

Zahrun commented Jul 7, 2022

Will this still cause interfaces to be announced when they come up later? Is there a periodic check for new addresses? Sorry I didn't have a chance to look at the code yet.

I think lib/connections/service.go manages changes in network configurations, but maybe someone who knows can confirm

@bt90
Copy link
Contributor

bt90 commented Jul 8, 2022

It should. How would address changes be picked up otherwise?

@acolomb
Copy link
Member

acolomb commented Jul 8, 2022

I assumed it's implemented like that. Just sometimes asking the obvious questions uncovers things everybody assumed but are actually not true. Having glanced at the diff now I'm sure at least this PR doesn't affect that behavior in any way.

@imsodin
Copy link
Member

imsodin commented Jul 8, 2022 •
edited

Good point!
(What's obvious and trivial about this?)
The global discovery service (not connections service) does announcements on a timer. When it does the announcements it asks for all the addresses, which then calls this function. So bringing the interface up won't do anything directly (because we don't monitor that), but it will be picked up quickly as global announcements happen very frequently (5s).

@calmh calmh added this to the v1.20.4 milestone Jul 12, 2022
calmh added a commit to calmh/syncthing that referenced this pull request Jul 21, 2022
@calmh
Merge branch 'main' into owners2
013b29e 
* main:
  gui, man, authors: Update docs, translations, and contributors
  cmd/syncthing/cli: Add show discovery command (fixes syncthing#8007) (syncthing#8378)
  gui, man, authors: Update docs, translations, and contributors
  lib/osutil: Only announce address of interfaces which are up (fixes syncthing#7458) (syncthing#8422)
  gui: Fix missing span end tag and missing nbsp semicolon in HTML (syncthing#8419)
calmh added a commit to calmh/syncthing that referenced this pull request Jul 28, 2022
@calmh
Merge branch 'main' into xattr
da3fe7a 
* main:
  cmd/syncthing, lib/config: Remove restartOnWakeup option & functionality (fixes syncthing#8448) (syncthing#8449)
  gui: Remove blank meta tags (syncthing#8362)
  gui: Add device sync status (fixes syncthing#7981) (syncthing#8401)
  gui: Fix detailed staggered versioning information in folder info (ref syncthing#8348) (syncthing#8433)
  all: Support syncing ownership (fixes syncthing#1329) (syncthing#8434)
  gui, man, authors: Update docs, translations, and contributors
  lib/model, lib/config: Apply sensible defaults for auto-accepted encrypted folder (fixes syncthing#8296) (syncthing#8427)
  gui: Move filesystem watcher explanation from tooltip to help block (syncthing#8432)
  gui: Use discovered IDs from cache when adding a new remote device (syncthing#8382)
  build: Update goleveldb (syncthing#8440)
  gui, man, authors: Update docs, translations, and contributors
  cmd/syncthing/cli: Add show discovery command (fixes syncthing#8007) (syncthing#8378)
  gui, man, authors: Update docs, translations, and contributors
  lib/osutil: Only announce address of interfaces which are up (fixes syncthing#7458) (syncthing#8422)
  gui: Fix missing span end tag and missing nbsp semicolon in HTML (syncthing#8419)
@st-review st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Jul 7, 2023
@syncthing syncthing locked and limited conversation to collaborators Jul 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@calmh calmh calmh approved these changes

Assignees
No one assigned
Labels
frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion
Projects
None yet
Milestone
v1.20.4
Development

Successfully merging this pull request may close these issues.

Syncthing announces address of interface which is down
6 participants
@Zahrun @acolomb @bt90 @imsodin @calmh @st-review