-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Do a mDNS advertisement on minmdns server startup #4884
Changes from 18 commits
e08ed9b
67cd75b
8d72191
df6955d
86acb92
e28ffc6
c047193
d4b0efc
08d420e
756d32a
07e3e79
9fecbf9
e402b1d
2157ffc
c6127f6
8c91e4d
708772e
076431b
d285703
68527cf
fba34ab
88d1444
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,19 +33,17 @@ class QueryReplyFilter : public ReplyFilter | |
|
||
bool Accept(QType qType, QClass qClass, FullQName qname) override | ||
{ | ||
// resource type is ignored | ||
if ((mQueryData.GetType() != QType::ANY) && (mQueryData.GetType() != qType)) | ||
if (!AcceptableQueryType(qType)) | ||
{ | ||
return false; | ||
} | ||
|
||
if ((mQueryData.GetClass() != QClass::ANY) && (mQueryData.GetClass() != qClass)) | ||
if (!AcceptableQueryClass(qClass)) | ||
{ | ||
return false; | ||
} | ||
|
||
// path must match | ||
return mIgnoreNameMatch || (mQueryData.GetName() == qname); | ||
return AcceptablePath(qname); | ||
} | ||
|
||
/// Ignore qname matches during Accept calls (if set to true, only qtype and qclass are matched). | ||
|
@@ -59,9 +57,41 @@ class QueryReplyFilter : public ReplyFilter | |
return *this; | ||
} | ||
|
||
QueryReplyFilter & SetSendingAdditionalItems(bool additional) | ||
{ | ||
mSendingAdditionalItems = additional; | ||
return *this; | ||
} | ||
|
||
private: | ||
bool AcceptableQueryType(QType qType) | ||
{ | ||
if (mSendingAdditionalItems && mQueryData.IsBootAdvertising()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this check needed (isn't the 2nd condition true already?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IsBootAdvertising is only set during initial boot, not during normal query replies, where as 'sending additional items' is set for any reply processing. I believe they are both needed. |
||
{ | ||
return true; | ||
} | ||
|
||
return ((mQueryData.GetType() == QType::ANY) || (mQueryData.GetType() == qType)); | ||
} | ||
|
||
bool AcceptableQueryClass(QClass qClass) | ||
{ | ||
return ((mQueryData.GetClass() == QClass::ANY) || (mQueryData.GetClass() == qClass)); | ||
} | ||
|
||
bool AcceptablePath(FullQName qname) | ||
{ | ||
if (mIgnoreNameMatch || mQueryData.IsBootAdvertising()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need IsBootAdvertising instead of just using IgnoreNameMatch ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seemed to make the intent cleaner by having behaviour depend on query. The boot-time broadcaster builds a fake query (so that the sender sends replies to such a query) however it does not have access to the underlying filtering used by the mDNS sender. Alternative would be to propagate flags into the mdns sending and that seeemed to be a bit uglier. |
||
{ | ||
return true; | ||
} | ||
|
||
return (mQueryData.GetName() == qname); | ||
} | ||
|
||
const QueryData & mQueryData; | ||
bool mIgnoreNameMatch = false; | ||
bool mIgnoreNameMatch = false; | ||
bool mSendingAdditionalItems = false; | ||
}; | ||
|
||
} // namespace Minimal | ||
|
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.
Not a new issue, but doesn't seem right that if I rename my interface to "lollipop" it will be skipped.
Probably the name is the wrong thing to look at altogether, and we should just inspect the address.
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 could find no better way.
For devices this may not be an issue (since they control their wifi name and it is likely LWIP anyway). Would people rename interfaces on android/iOS?
Added a TODO to auto-create an issue.
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.
Can't you just get the address and see if it's ::1 or inside 127.0.0.0/8 ?
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.
InterfaceIterator does not provide the address only InterfaceAddressIterator does.
Absolutely possible to use AddressIterator instead and do some dedup, but I find it more bothersome than what I would be generally add as a 'fix if you are already touching this'.
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.
Real fix would probably to fix the interface iterator to be able to return an address iterator for that specified interface. Having 2 types of iterators that both iterate over all interfaces is strange.