-
Notifications
You must be signed in to change notification settings - Fork 220
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
Separately send large mDNS responses to comply with RFC 6762 #248
Conversation
… responses won't fit. This is required by section 17 of RFC6762. Make packet() into packets() and have it return a list of the packets that need to be sent to each of the interfaces.
…the question in the initial packet) and fixed the tests. In particular, the lots_of_names test's behaviour is explicitly supposed to change because the old behaviour was non-conforming.
…more and test coverage went down.
The "X" is now just a very minor code coverage reduction and the missed code coverage is related to the change. @jstasiak, what are your thoughts? It's pretty clear that the old implementation did not handle extra long packets with lots of responses, and I believe I've done a correct implementation of sending them out as multiple packets. I'm certain this fixes the ChromeCast Audios failing on my network. Thanks! |
…y test log_warning_once from test sutie.
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.
Thank you @gjbadros, it's a solid piece of work! I'll have another look tomorrow, but I think after my comments are addressed it's ready to be merged. I'd not worry about the logging method not being called and lowering the coverage slightly – it's just a number.
zeroconf/__init__.py
Outdated
@@ -852,7 +852,7 @@ def read_name(self) -> str: | |||
return result | |||
|
|||
|
|||
class DNSOutgoing: | |||
class DNSOutgoing(QuietLogger): |
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 don't remember the details about QuietLogger
precisely, but its methods are static so let's not inherit from it (I know some classes already do that – I'll remove it).
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.
Ok, I changed mine back but didn't change the others to avoid a merge conflict since it sounds like you're planning to fix those.
zeroconf/__init__.py
Outdated
"""Writes a record (answer, authoritative answer, additional) to | ||
the packet""" | ||
the packet. Returns True on success. """ |
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 you mention the failure scenarios in this docstring as well?
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.
Done.
zeroconf/__init__.py
Outdated
Generally, you want to use packets() in case the response | ||
does not fit in a single packet, but this exists for | ||
backward compatibility.""" | ||
p = self.packets() |
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 think it's fine to just call the variable packets
, not much saved by shortening it to just p
and it can help avoiding some confusion.
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.
Ok, I changed it; I don't like local variable names shadowing class instances/functions, but I get that other people don't like short variable names. My philosophy is that a variable names's length should be proportional to its scope and in this case the scope is tiny so I was comfortable with a single-letter variable name.
zeroconf/__init__.py
Outdated
return b'' | ||
|
||
def packets(self) -> List[bytes]: | ||
"""Returns a list of strings containing the packets' bytes |
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.
"bytestrings" instead of "strings"? "strings" implies text.
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.
Sure. I think I was just copying the former docstring language, but this is a good idea to be clear about.
self.insert_short(0, len(self.answers) - overrun_answers) | ||
self.insert_short(0, len(self.questions)) | ||
questions_written += 1 | ||
allow_long = True # at most one answer is allowed to be a long packet |
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.
Is it at most one answer per packet or per group of packets?
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.
Added docstring -- multiple answers per packet are fine. It's just the case of having so many answers that it overflows the smaller 1470ish MTU of ethernet -- in that scenario, the application layer MUST send separate application-level responses rather than rely on IP fragmentation to break up a bigger packet. The only exception is when a single answer does not fit inside the smaller MTU in which case it's allowed to be bigger. I suspect that scenario will still crash ChromeCast Audios, but that situation is exceedingly rare, whereas many responses that exceed 1470 bytes happens within a large but not crazy set of devices.
zeroconf/test.py
Outdated
@@ -374,7 +381,7 @@ def on_service_state_change(zeroconf, service_type, state_change, name): | |||
|
|||
# wait until the browse request packet has maxed out in size | |||
sleep_count = 0 | |||
while sleep_count < 100 and longest_packet_len < r._MAX_MSG_ABSOLUTE - 100: | |||
while sleep_count < 100 and longest_packet_len < (5 * r._MAX_MSG_TYPICAL) - 100: |
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.
Does 5 have some specific meaning in this context?
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.
No, I changed it back since it's really immaterial now, and I explain why it doesn't matter in a comment.
…e bit more documentation, a variable rename, and a better explanation of a test.
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.
LGTM! I'd like to merge it after #258 is merged and released.
Yep understood. I'm glad that issue isn't comingled with this change too.
Thanks!
…On Mon, May 25, 2020, 2:55 PM Jakub Stasiak ***@***.***> wrote:
***@***.**** approved this pull request.
LGTM! I'd like to merge it after #258
<#258> is merged and
released.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#248 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AALOHTKTTS6244XH7IZHSJ3RTLSMVANCNFSM4NAAT7KQ>
.
|
- DNSOutgoing.packet only returned a partial message when the DNSOutgoing contents exceeded _MAX_MSG_ABSOLUTE or _MAX_MSG_TYPICAL This was a legacy function that was replaced with .packets() which always returns a complete payload in python-zeroconf#248 As packet() should not be used since it will end up missing data, it has been removed
- DNSOutgoing.packet only returned a partial message when the DNSOutgoing contents exceeded _MAX_MSG_ABSOLUTE or _MAX_MSG_TYPICAL This was a legacy function that was replaced with .packets() which always returns a complete payload in #248 As packet() should not be used since it will end up missing data, it has been removed
This fixes issue #245
Split up large multi-response packets into separate packets instead of relying on IP Fragmentation. IP Fragmentation of mDNS packets causes ChromeCast Audios to crash their mDNS responder processes and RFC 6762
(https://tools.ietf.org/html/rfc6762) section 17 states some
requirements for Multicast DNS Message Size, and the fourth paragraph reads:
"A Multicast DNS packet larger than the interface MTU, which is sent
using fragments, MUST NOT contain more than one resource record."
This change makes this implementation conform with this MUST NOT clause.