Skip to content

extmod/modnetwork.h: Increase hostname limit to 64 bytes #12403

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

Closed
wants to merge 1 commit into from

Conversation

M3t0r
Copy link

@M3t0r M3t0r commented Sep 11, 2023

PR #10635 introduced this limit which seems rather arbirtrary and wildly insufficient to me.

A 16 bytes length limits means a 15 character string limit, which even ubinascii.hexlify(machine.unique_id()) on most ports will break. And then assuming you just supply a random 15 byte hex string, most people won't be able to identify the hosts function in a list on their wifi routers.

I wanted to set my hostname to rpi-fan-controller-{hex_uid} and just got a ValueError without explanation. It took me 2 hours to figure what went wrong. Because of this I added a note about the exception and limitation to the documentation.

Since I didn't really understand the reason for 16 bytes I set the new limit to the size used in all involved protocols: DHCP 64 bytes, DNS 63 "octets" (characters) plus length byte, and mDNS which points to the DNS RFC. I can imagine that some ports for small boards might want to decrease the size of this statically allocated buffer, and from what I gathered from the C code it would be a simple as defining MICROPY_PY_NETWORK_HOSTNAME_MAX_LEN, so I mentioned this possibility for end users in the documentation as well.

PR micropython#10635 introduced this limit which seems rather arbitrary and wildly
insufficient to me.

A 16 bytes length limits means a 15 character string limit, which even
`ubinascii.hexlify(machine.unique_id())` on most ports will break. And
then assuming you just supply a random 15 byte hex string, most people
won't be able to identify the hosts function in a list on their wifi
routers.

I wanted to set my hostname to `rpi-fan-controller-{hex_uid}` and just
got a ValueError without explanation. It took me 2 hours to figure what
went wrong. Because of this I added a note about the exception and
limitation to the documentation.

Since I didn't really understand the reason for 16 bytes I set the new
limit to the size used in all involved protocols: DHCP 64 bytes, DNS 63
"octets" (characters) plus length byte, and mDNS which points to the DNS
RFC. I can imagine that some ports for small boards might want to
decrease the size of this statically allocated buffer, and from what I
gathered from the C code it would be a simple as defining
`MICROPY_PY_NETWORK_HOSTNAME_MAX_LEN`, so I mentioned this possibility
for end users in the documentation as well.

Signed-off-by: Simon Lutz Brüggen <micropython@m3t0r.de>
@M3t0r M3t0r force-pushed the increase-hostname-limit branch from 08571fb to 72b1934 Compare September 11, 2023 01:19
@github-actions
Copy link

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:   +48 +0.012% PYBV10[incl +48(data)]
     mimxrt:   +48 +0.013% TEENSY40[incl +48(data)]
        rp2:    +0 +0.000% RPI_PICO
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS

@codecov
Copy link

codecov bot commented Sep 11, 2023

Codecov Report

Merging #12403 (72b1934) into master (3637252) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #12403   +/-   ##
=======================================
  Coverage   98.38%   98.38%           
=======================================
  Files         158      158           
  Lines       20933    20933           
=======================================
  Hits        20595    20595           
  Misses        338      338           

@jimmo
Copy link
Member

jimmo commented Sep 11, 2023

@M3t0r Thanks for the PR, this seems pretty reasonable to me. Thanks for the additional detail about DHCP/mDNS max lengths.

In #10635 I took the existing limit from (iirc) the cyw43 driver, but didn't realise that esp32 had a higher limit, so yes this is definitely a regression.

An additional 48 bytes of .data is not an entirely trivial decision. The other option would be to make this string heap-allocated instead. The way that would probably work is to have a default hostname in ROM and a (root pointer) const char* pointing to an optionally heap allocated version to override it. So compared to a 64 byte buffer like this PR, it would be a RAM saving of 60 bytes for the case where someone uses the default hostname, and a small overhead of 16 bytes for the string object. So overall a win for anyone using a hostname shorter than 48 characters.

@jimmo
Copy link
Member

jimmo commented Sep 11, 2023

See #12404 for an implementation of the above comment.

@M3t0r
Copy link
Author

M3t0r commented Sep 11, 2023

Yeah, that does sound a whole lot better 😄 Let's go with #12404 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants