-
Notifications
You must be signed in to change notification settings - Fork 124
Add rudimentary support for broadcast via aiosync-client #292
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
base: master
Are you sure you want to change the base?
Changes from all commits
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 |
---|---|---|
|
@@ -140,7 +140,7 @@ async def _append_tokenmanaged_transport(self, token_interface_constructor): | |
self.request_interfaces.append(tman) | ||
|
||
@classmethod | ||
async def create_client_context(cls, *, loggername="coap", loop=None, transports: Optional[List[str]] = None): | ||
async def create_client_context(cls, *, loggername="coap", loop=None, transports: Optional[List[str]] = None, broadcast: bool = False): | ||
"""Create a context bound to all addresses on a random listening port. | ||
|
||
This is the easiest way to get a context suitable for sending client | ||
|
@@ -159,7 +159,7 @@ async def create_client_context(cls, *, loggername="coap", loop=None, transports | |
if transportname == 'udp6': | ||
from .transports.udp6 import MessageInterfaceUDP6 | ||
await self._append_tokenmanaged_messagemanaged_transport( | ||
lambda mman: MessageInterfaceUDP6.create_client_transport_endpoint(mman, log=self.log, loop=loop)) | ||
lambda mman: MessageInterfaceUDP6.create_client_transport_endpoint(mman, log=self.log, loop=loop, broadcast=broadcast)) | ||
elif transportname == 'simple6': | ||
from .transports.simple6 import MessageInterfaceSimple6 | ||
await self._append_tokenmanaged_messagemanaged_transport( | ||
|
@@ -241,13 +241,11 @@ async def create_server_context(cls, site, bind=None, *, loggername="coap-server | |
lambda mman: MessageInterfaceSimple6.create_client_transport_endpoint(mman, log=self.log, loop=loop)) | ||
elif transportname == 'tinydtls': | ||
from .transports.tinydtls import MessageInterfaceTinyDTLS | ||
|
||
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. Please don't change whitespace or other unrelated code in PRs. If this is your editor's doing, you may want to reign it in on code bases that don't follow that editor's particular coding style. 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. Yep, sorry 'bout that, this one (or two) slipped through. I managed to avoid my urge to run black against the entire codebase and submit that as a PR 😜 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. The concern is duly noted, and as soon as I have the number of large branches down a bit, and have verified that no code is becoming less readable, black will be the one color in which you get aiocoap, and CI will mark any specks of color with a big red cross mark. 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.
The world wants to know- when will we have PEP-484 across the codebase and no red marks next to mypy? 😉 On a serious note though, I'd be happy to help with that effort- for the public interfaces or the entire project- if you're interested obviously, and if time permits Personally I now find it nearly impossible to write good interfaces without it, though that may be evidence of my lack of discipline rather than evidence of the broad value of type annotations |
||
await self._append_tokenmanaged_messagemanaged_transport( | ||
lambda mman: MessageInterfaceTinyDTLS.create_client_transport_endpoint(mman, log=self.log, loop=loop)) | ||
# FIXME end duplication | ||
elif transportname == 'tinydtls_server': | ||
from .transports.tinydtls_server import MessageInterfaceTinyDTLSServer | ||
|
||
await self._append_tokenmanaged_messagemanaged_transport( | ||
lambda mman: MessageInterfaceTinyDTLSServer.create_server(bind, mman, log=self.log, loop=loop, server_credentials=self.server_credentials)) | ||
elif transportname == 'simplesocketserver': | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -256,7 +256,13 @@ def request(self, request): | |
self.outgoing_requests[key] = request | ||
request.on_interest_end(functools.partial(self.outgoing_requests.pop, key, None)) | ||
|
||
''' | ||
'''Not implemented | ||
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. See above on coding style. |
||
def multicast_request(self, request): | ||
return MulticastRequest(self, request).responses | ||
''' | ||
|
||
'''Not implemented | ||
def broadcast_request(self, request): | ||
return MulticastRequest(self, request).responses | ||
''' | ||
|
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -247,11 +247,20 @@ def _local_port(self): | |||||||
return self.transport.get_extra_info('socket').getsockname()[1] | ||||||||
|
||||||||
@classmethod | ||||||||
async def _create_transport_endpoint(cls, sock, ctx: interfaces.MessageManager, log, loop, multicast=[]): | ||||||||
async def _create_transport_endpoint(cls, sock, ctx: interfaces.MessageManager, log, loop, multicast=[], broadcast: bool = False): | ||||||||
try: | ||||||||
sock.setsockopt(socket.IPPROTO_IPV6, socknumbers.IPV6_RECVPKTINFO, 1) | ||||||||
except NameError: | ||||||||
raise RuntimeError("RFC3542 PKTINFO flags are unavailable, unable to create a udp6 transport.") | ||||||||
if broadcast is True: | ||||||||
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. See above: Can this be unconditional? 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. 🤞 |
||||||||
try: | ||||||||
# This shouldn't be needed, at least not when using --broadcast | ||||||||
# with aiocoap-client. | ||||||||
if not sock.getsockopt(socket.SOL_SOCKET, socket.SO_BROADCAST): | ||||||||
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. Checking and then setting when not yet set is no faster or better than just setting it. |
||||||||
sock.setsockopt(socket.SOL_SOCKET, socket.SO_BROADCAST, 1) | ||||||||
except Exception: | ||||||||
log.fatal("Unable to set socket to SO_BROADCAST; setsockopt() failed") | ||||||||
raise | ||||||||
Comment on lines
+262
to
+263
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.
Suggested change
When there is no extra flag, this can best-effort. |
||||||||
if socknumbers.HAS_RECVERR: | ||||||||
sock.setsockopt(socket.IPPROTO_IPV6, socknumbers.IPV6_RECVERR, 1) | ||||||||
# i'm curious why this is required; didn't IPV6_V6ONLY=0 already make | ||||||||
|
@@ -299,9 +308,15 @@ async def _create_transport_endpoint(cls, sock, ctx: interfaces.MessageManager, | |||||||
return protocol | ||||||||
|
||||||||
@classmethod | ||||||||
async def create_client_transport_endpoint(cls, ctx: interfaces.MessageManager, log, loop): | ||||||||
async def create_client_transport_endpoint(cls, ctx: interfaces.MessageManager, log, loop, broadcast: bool = False): | ||||||||
sock = socket.socket(family=socket.AF_INET6, type=socket.SOCK_DGRAM) | ||||||||
sock.setsockopt(socket.IPPROTO_IPV6, socket.IPV6_V6ONLY, 0) | ||||||||
if broadcast is True: | ||||||||
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. This is redundant with the code in _create_transport_endpoint, which is called right after this. |
||||||||
try: | ||||||||
sock.setsockopt(socket.SOL_SOCKET, socket.SO_BROADCAST, 1) | ||||||||
except Exception: | ||||||||
log.fatal("Unable to set socket to SO_BROADCAST; setsockopt() failed") | ||||||||
raise | ||||||||
|
||||||||
return await cls._create_transport_endpoint(sock, ctx, log, loop) | ||||||||
|
||||||||
|
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 there any good reason to make this a flag? IOW: Will anything bad come of aiocoap always setting that option?
We're setting a lot of socket options even though we don't know yet whether we'll need them on this particular occasion. (For example, V6ONLY=0 is set even though we may not use IPv4 at all).
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.
Very good question. I think the downside is that superuser privileges (or possibly some CAP_* on Linux) are required to set this particular ioctl, but I need to check on that
Uh oh!
There was an error while loading. Please reload this page.
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 I'm misremembering regarding the requirement of additional privileges, the EPERM was coming from an earlier version of this patch which also added SO_BINDTODEVICE
You may wonder why on earth that would be necessary- the server software I was dealing with actually required the universal broadcast address 255.255.255.255 for a specific endpoint, and I'm working in a multi-homed environment with multiple interfaces having +BROADCAST
In the end I decided it would be outside the scope of this PR to add an option to force the interface, especially without any discussion. My final conclusion was to not attempt to solve that problem with changes to aiocoap; there are a few workarounds good enough for that exceptional case
tl; dr; your suggestion on defaulting to SO_BROADCAST seems to be a good one as far as I can tell