Add support for unicode in SSIDs and PSKs #15
Conversation
The corresponding c code for encoding and decoding them can be found in wpa_supplicant/src/utils/common.c printf_decode escapes a SSID for returning in scan results wpa_config_parse_string reads a string from the config or the socket. It might be nicer to use the printf encoded P"I \xf0\x9f\xa6\x80 rust" style but I could not find a decent encoder.
|
There are probably still issues as it is possible to have an SSID that is not valid unicode, but I don't know support that without breaking the API. I did not check the access point side of things so it is possible that there are more places there that need to deal with encoding better. I feel slightly dirty overriding clippy about using actual tabs in the example in the doc test. Let me know if there are any issue or changes you would like. |
|
@aj-bagwell i'm really sorry i missed this on the way in. my GH notifications were messed up for a bit this summer. i'll give this proper review in the coming week |
src/ap/types.rs
Outdated
| } | ||
| } | ||
|
|
||
| fn deserialize_i32<'de, D>(deserializer: D) -> std::result::Result<i32, D::Error> |
There was a problem hiding this comment.
why do we need a custom i32 deserializer at all?
There was a problem hiding this comment.
This is needed because I didn't implement a proper Deserializer for the config format, instead I just convert the key=value pairs into a HashMap<&str, String> and pass it off to MapDeserializer to do the serde magic, the downside is that all the values are Strings so numbers need to be annotated with #[serde(deserialize_with = "deserialize_i32")] to then parse the String to a i31.
I did not mean to push the last commit into this pull request, as I had not tested it with the AP mode stuff properly and looking at it again there is a bigger issue is as I have not implemented the Vec part of the Deserializer either so stuff like ssid[0]=Wi-Fi is not going to work.
I will go off and sort out a proper Deserializer.
However today I went and collected some example responses from hostapd, and I'm not sure that the existing structures work with what it returns, as apart from the ket_mgmt/key_mgmt typo it looks like some of the fields are optional, so even with the the old INI config parser I am getting serde errors.
What do you recommend?
- Change the structs to make them
Option<String>(there are some other types that could be better like frequency) - Leave them as is and keep returning an error when hostapd does not e.g.
wpa_pairwise_cipher - fill in the empty string for missing responses
There was a problem hiding this comment.
I think the Option route is best, IMO. If you can collect a few examples to write unit tests against that would be ideal. I'll try to gather a few samples on some test systems this week as well.
In general, you've done a great job isolating the deserializing better so it'll be a good place to work from.
There was a problem hiding this comment.
Pushed an update with a config parser with a proper Deserializer that now handles bss[0] style keys. Followed by a commit that changes the types of ap::Status and ap::Config to match the results that I get and line up with the logic in the hostapd code. Is is not backwards compatible and probably calls for a semver bump.
There was a problem hiding this comment.
We're running below version 1 so I'm okay making whatever API changes. The rule as I've understood is that just bumping 0.x by one is acceptable.
Generally, I'm a big fan of how you've restructured things and this looks really good. I hope to have some time to actually play with it and run it in the next few days.
aj-bagwell
force-pushed
the
unicode
branch
2 times, most recently
from
488f6bd to
98f6bf7
Compare
src/error.rs
Outdated
| impl Clone for Error { | ||
| fn clone(&self) -> Self { | ||
| match self { |
There was a problem hiding this comment.
This all needs to be implemented by hand just because io::Error doesn't implement clone right? What about just dropping io::Error into an Arc and then deriving clone?
There was a problem hiding this comment.
Basically yes io::Error and SendError don't implement Clone.
Arc is an interesting idea, I had a play with it this morning, and what I gain from deriving Clone I loose from having to manually implement From and manually wrapping UnsolicitedIoError with Arc. The Arc version is still less code but it bugs me to add a whole level of indirection and atomic counter wrapped around what it probably just an i32.
The reason I added Clone is that I thought it made more sense to deal with error as the response to the client, rather than crashing the main worker task, and so I need to forward all scan errors to all the callers awaiting scan results. My other idea is forward the error to the the first client, then retry getting the scan results so there is no need to clone the error.
I'll add a commit without the cloning and you can see if you think that is nicer.
There was a problem hiding this comment.
So funny enough I've had the same philosophical debate at work recently about io::Errors and fundamentally, there are reasons why they're not clonable already. Within this particular context, I'd boil it down to the following idea: all of these clients are separated by channels from the original io::Error and can't do anything with the cloned error. It makes no sense that N client could get the io::Error when they can't do anything about it.
You've done a lot of good things in this PR and I'm happy to move along and land it if you make a simple change: create a new Error variant called Error::RemoteIo and your io_clone fn could be
fn clone_io(err: &std::io::Error) -> Error {
Error::RemoteIo(err.to_string())
}
This is a bit hacky but gets the job done... We just have to be careful about who gets the cloned error because we haven't really cloned it by changing the enum variant of it 😬
Alternatively, if you have the appetite to try something more in depth, I'm not sure the clients calls should return an Error that contains io::Error. The Error enum might benefit by being separated out so that some Errors apply to the runtimes (WifiAp::run / WifiStation::run) and some apply to the client calls. So this should take a little more time untangling the monolithic error but I think it will have a nice benefit of being clear about what calls actually provide which errors. Plus we haven't butchered the idea of what clone is by mutated the enum variant.
There was a problem hiding this comment.
Actually, please don't do what I suggested in cloning. Perhaps a fair half-measure is doing: "error.for_client()" where you do something like the above. I don't like the idea of having a clone footgun laying around
There was a problem hiding this comment.
So for now I have side stepped this with b2eaace meaning I don't need to clone errors anymore, as each error goes to exactly one client.
However I think you have a point around breaking up the giant Error enum and thinking more carefully which errors go where and what the client should do about them.
I glibly decided to forward them back to the client as the errors I was seeing where parsing errors, (I didn't have good enough logs to work out what was failing to be parsed) so the client could deal with the failed request but keep using the connection, however an IO error would imply that something is wrong with the socket so maybe it does make more sense for the runner to fail at that point as the socket probably need reconnecting. That said even for IO errors the client still need some kind of error as otherwise it will hang forever waiting for a response that never comes.
How about I unpick my error handling changes from the UTF8 escaping changes and stick them in a separate PR, then in that PR I have an attempt at creating separate enums for client and runner errors.
This replaces the INI config parser from the config crate with a simplified parser that only support k=v pairs, but does proper printf unsecaping for non printable/on ASCII characters.
Some of the fields of status are optional, so mark them as such so that serde does not fail when they are missing. Most fields are numbers so convert them to u64. I did some prelimeninary poking around the hostapd 2.10 source code to see what is optional. It is possible older versions may not return all fields.
The corresponding c code for encoding and decoding them can be found in wpa_supplicant/src/utils/common.c
printf_decode escapes a SSID for returning in scan results
wpa_config_parse_string reads a string from the config or the socket. It might be nicer to use the printf encoded P"I \xf0\x9f\xa6\x80 rust" style but I could not find a decent encoder.