Essid support#22
Conversation
nikeflight
left a comment
nikeflight
left a comment
There was a problem hiding this comment.
Needs unit tests as well.
| unescaped_chars = HEX_ESCAPE_RE.sub(lambda m: chr(int(m.group(1), 16)), essid) | ||
| return unescaped_chars.encode("latin-1") | ||
|
|
||
| def build_ssid_value(raw_essid_from_iwlist: str) -> str: |
There was a problem hiding this comment.
This variable name implies we should be getting the essid from iwlist.
But you're passing the ssid from the piaware-config.txt, does that make sense?
Are people going to be entering their essid as hex-escaped bytes?
There was a problem hiding this comment.
Could you change this method name to something more descriptive like hex_to_string or something similar.
There was a problem hiding this comment.
This variable name implies we should be getting the essid from iwlist. But you're passing the
ssidfrom the piaware-config.txt, does that make sense?Are people going to be entering their
essidas hex-escaped bytes?
Similar to the comment below. Please check how piaware-configurator works with piaware-support in the WIFI BLE setup.
This PR will not affect how SSID was processed when people enter their SSID in piaware-config.txt the old way.
I plan to create another branch for unit tests. |
Why? Unless there's a good reason, you generally want to keep unit tests in the same PR they're testing. It also makes code review easier. This PR is still small so adding in unit tests isn't really an issue. |
Because I want to get it fixed first for the release. It will take me some time to understand the unit test script and write the test cases. |
mutability
left a comment
mutability
left a comment
There was a problem hiding this comment.
Let me put together some code to show what I mean.
Also, have you verified that the whole list of pathological testcases I put in Confluence work correctly? I don't see them all in the unit tests and from eyeballing the current code I think some will fail.
| if is_escaped_essid(raw_essid_from_iwlist): | ||
| raw_bytes = unescape_regex(raw_essid_from_iwlist) | ||
| return bytes_to_nm_ssid_value(raw_bytes) | ||
| return raw_essid_from_iwlist |
There was a problem hiding this comment.
as mentioned in the previous PR, you need to handle the case where the SSID looks like a decimal-octet-string such as 101;
| def is_escaped_essid(essid: str) -> bool: | ||
| """True if the ESSID contains at least one \\xHH hex escape sequence.""" | ||
| return HEX_ESCAPE_RE.search(essid) is not None | ||
|
|
||
| def bytes_to_nm_ssid_value(raw: bytes) -> str: | ||
| return ";".join(str(b) for b in raw) + ";" | ||
|
|
||
| def unescape_regex(essid: str) -> bytes: | ||
| """ | ||
| Only touch \\xHH sequences with a targeted regex substitution, leave | ||
| every other character exactly as-is, then encode the result as Latin-1 | ||
| (a 1:1 char -> byte mapping) to recover the raw bytes. | ||
| """ | ||
| unescaped_chars = HEX_ESCAPE_RE.sub(lambda m: chr(int(m.group(1), 16)), essid) | ||
| return unescaped_chars.encode("latin-1") | ||
|
|
||
| def escaped_hex_to_bytes_string(raw_essid_from_iwlist: str) -> str: | ||
| """Return the exact text to put after 'ssid=' in the .nmconnection file.""" | ||
| if is_escaped_essid(raw_essid_from_iwlist): | ||
| raw_bytes = unescape_regex(raw_essid_from_iwlist) |
There was a problem hiding this comment.
This technically works but it's a bit of a case of getting the right answer for the wrong reason.
ideally what we want to do is:
- take the raw ESSID and turn it into bytes
- take those bytes and represent them in a way that networkmanager understands
the current code muddies the waters by basing the decisions in (2) on the format of the raw ESSID, not the bytes it represents.
I'll put together some code to do this a bit more cleanly.
|
For some context here, I asked @deadlywang to fix the specific case exhibited by the BLE setup resulting in the SSID of "Erick\xE2\x80\x99s iPhone" for this release... I thought broadening the fix to all SSID edge cases might be a bit risky if it ends up touching multiple parts of the networking stack, and can be something we tackle in a follow-on release. That said, if the team feels we can safely generalize the fix with contained changes and solid testing, I’m supportive of exploring that path. |
|
I mean, to be frank, there's no way this is mature enough that we should be building a release candidate from it. It does need tests and further cleanup. I'm also not happy about the double-escaping that's going to go on if we use the Can we push this to 11.2 or later please. |
4 participants
Targeted support for ESSID generated by iwlist.
Specifically the case for wifi hotspots showing as "Erick\xE2\x80\x99s iPhone" in piaware-config.txt