Add bindings for querying and setting Android System Properties #495
Conversation
MarijnS95
force-pushed
the
system-properties
branch
from
5a82ecb to
378e710
Compare
| /// | ||
| /// # Deprecation | ||
| /// Deprecated since Android O (API level 26), use [`Property::find()`] with | ||
| /// [`Property::read_callback()`] instead which does not have a limit on `value` nor `name` length. |
There was a problem hiding this comment.
Should the docs here maybe also mention the limit on what the limit on returned value is, like is done for the property name?
There was a problem hiding this comment.
That's already covered in the first paragraph / "Returns xxx title" of this function?
Perhaps I should remove and `name` here as we've never introduced a limit on name length in this function and as demonstrated elsewhere, there is none for get() (only for __system_property_read()).
ndk/src/system_properties.rs
Outdated
| // TODO: should we return the name of ffi::PROP_NAME_MAX? | ||
| ffi::__system_property_read(self.0.as_ptr(), std::ptr::null_mut(), value) |
There was a problem hiding this comment.
Notice that I still have to consider whether to always allocate + move a name here in read_raw() and read().
This may seem silly when using let prop = Property::find(c"we.know.the.name"), but it's the only API to get names out of Property::foreach(|prop| prop.how_to_get_the_name()) before Android O where one can call prop.read_callback() and get the name (without limitations on length).
There was a problem hiding this comment.
The most nice and trivial implementation uses an initialized slice of ffi::PROP_NAME_MAX characters, so that we can safely call CStr::from_bytes_until_nul() and copy into a host allocation. But that requires MSRV 1.69, which must be an acceptable bump but I still haven't set up a proper MSRV policy to follow (besides chugging along with winit).
There was a problem hiding this comment.
Replaced this with an ordinary call to CStr::from_ptr() on a MaybeUninit slice, like we do in other places where I described to use CStr::from_bytes_until_nul() when the MSRV is bumped.
Notice also that this doesn't require a NulError for the name, and in hindsight I'm considering to remove it for the value too: the contract with the NDK specifies that it should have written a NUL terminator within the property length limit, and returned the length of the string (excluding NUL terminator). If that's violated, which should pretty much never happen, we might as well unwrap and panic rather than bubbling a "pretty much unreachable" error up to the user to match in thiserror. They will after all want to match on MissingOrEmpty and handle that gracefully.
At least that's the guidance I've been following over the years when deciding to prefer returning Result::Err over a panic!().
MarijnS95
force-pushed
the
system-properties
branch
from
378e710 to
54887d7
Compare
MarijnS95
force-pushed
the
system-properties
branch
6 times, most recently
from
2611db6 to
10d0d78
Compare
The Android System Property API might not be very useful for most apps even though it still provides a read-only view of all properties but no rights to set any. Since this is accessible via the NDK this crate should provide a clean and complete (including UB-free) wrapper implementation either way. This requires bumping `thiserror` to `1.0.29` which introduced proper impl bounds in https://togithub.com/dtolnay/thiserror/pull/148 such that no `Debug` bound needs to be written anywhere that `GetError<>` is used. As a second advantage, this error type can still be used with `FromStr` implementations whose `Err` type does not implement `Error`/`Debug`, those just cannot be `.unwrap()`ped.
MarijnS95
force-pushed
the
system-properties
branch
from
10d0d78 to
9c63222
Compare
The Android System Property API might not be very useful for most apps even though it still provides a read-only view of all properties but no rights to set any. Since this is accessible via the NDK this crate should provide a clean and complete (including UB-free) wrapper implementation either way.