-
Notifications
You must be signed in to change notification settings - Fork 950
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.
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
mdns: MdnsConfig derives Clone and Debug. #2007
Conversation
801f6c1
to
1b4a18e
Compare
swarm/src/lib.rs
Outdated
self.behaviour.inject_new_external_addr(&a); | ||
self.external_addrs.add(a, s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.behaviour.inject_new_external_addr(&a); | |
self.external_addrs.add(a, s) | |
let result = self.external_addrs.add(a, s); | |
if let AddAddressResult::Inserted = &result { | |
self.behaviour.inject_new_external_addr(&a); | |
} | |
result |
swarm/src/lib.rs
Outdated
self.behaviour.inject_expired_external_addr(addr); | ||
self.external_addrs.remove(addr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.behaviour.inject_expired_external_addr(addr); | |
self.external_addrs.remove(addr) | |
if self.external_addrs.remove(addr) { | |
self.behaviour.inject_expired_external_addr(addr); | |
} |
However, note also that the most common way in which external addresses expire is as a result of new addresses being added. See https://github.com/libp2p/rust-libp2p/blob/master/swarm/src/registry.rs#L186-L196. So, to be able to properly invoke this callback whenever an external address expires, I think you would need to incorporate the expired/removed addresses within a call to add()
in the AddAddressResult
, so that the Swarm
can then invoke the callback for each.
The additions generally look good to me. We just need to make sure that the new callbacks are called only (and always) when appropriate. |
/// Configuration for mDNS. | ||
#[derive(Clone, Debug)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you extract this change to a separate pull request? Making only a single cohesive change per pull request makes maintaining this project a lot easier, especially around version and changelog management.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you renamed the PR, I moved the api changes, as this one has the branch name mdns-config
Towards #2006 unblocks a new ipfs-embed release.