Ensure Windows code is correct

[Arcterus]

The Windows code is way more complicated than the Redox and Unix code for a variety of reasons. We need to verify that the code is compatible with uname from GNU coreutils (or most GNU ports). Additionally, we need to check that all the code is indeed safe as it is filled with calls to the Windows API.

[sylvestre]

@rivy I guess we can fix this issue now, right ? :)

[rivy]

I think so... But I didn't review the existing code very closely so I don't have a high level of confidence. I'll review the code a bit more closely and get back to you.

I do think it can be released in the current form.

[sylvestre]

Done:

Uploading platform-info v1.0.2

https://crates.io/crates/platform-info/1.0.2

[rivy]

I found at least one problem... nodename() is returning a string with a trailing NUL (which should have been trimmed).

[rivy]

@sylvestre , I'm working on changes and testing for the WinOS code.

While implementing the changes, I'm struck that the API should really be returning OSString's instead of String's, letting the users convert to String's when desired/needed. This would address Issue #6 as well.

It would entail a semver bump to v2.0.

What do you think?

[sylvestre]

i don't like semver bump but if you think it makes sense, let's do it :)

[rivy]

Here's my suggested new API:

pub trait Uname {
    /// The name of this implementation of the operating system.
    fn sysname(&self) -> Result<Cow<str>, &OsString>;

    /// The node name (network node hostname) of this machine.
    fn nodename(&self) -> Result<Cow<str>, &OsString>;

    /// The current release level of the operating system.
    fn release(&self) -> Result<Cow<str>, &OsString>;

    /// The current version level of the current release.
    fn version(&self) -> Result<Cow<str>, &OsString>;

    /// The name of the current system's hardware.
    fn machine(&self) -> Result<Cow<str>, &OsString>;

    /// The name of the current OS.
    fn osname(&self) -> Result<Cow<str>, &OsString>;
}

In-use would be:

// examples/ex.rs
// * use `cargo run --example ex` to execute this example

use platform_info::*;

fn main() {
    let uname = PlatformInfo::new().unwrap();
    println!("{}", uname.sysname().unwrap_or_else(|os_s| os_s.to_string_lossy()));
    println!("{}", uname.nodename().unwrap_or_else(|os_s| os_s.to_string_lossy()));
    println!("{}", uname.release().unwrap_or_else(|os_s| os_s.to_string_lossy()));
    println!("{}", uname.version().unwrap_or_else(|os_s| os_s.to_string_lossy()));
    println!("{}", uname.machine().unwrap_or_else(|os_s| os_s.to_string_lossy()));
    println!("{}", uname.osname().unwrap_or_else(|os_s| os_s.to_string_lossy()));
}

It enables simple access to the uname results, but still allows fall-back access to the underlying OsString if/when needed. Results with well-formed, UTF-8 compatible, strings should be efficient (no-copy).

I've got a working draft (emphasis on draft with lots of extraneous notes) at https://github.com/rivy/rs.platform-info/compare/main...wip. I'm still working on checking the WinAPI functions, minimizing other unsafe code, and removing undefined behavior.

[tertsdiepraam]

I don't see how that's better than just returning &OsStr. Users of the library can always to do their own conversions based on their needs (e.g. to_str if precision is required, to_string_lossy for printing). Often those turn out to be clearer and more concise than than what you're proposing.

[rivy]

@tertsdiepraam , TLDR; Yep, I agree with you. I'll switch to an API returning &OsStr's.

FYI, I modelled the initial API after OsString::into_string() -> Result<String, OsString> in std. But, after reading your comment and doing some experimentation, I think that you're correct.

It would simplify some platform-info code. There is a trade-off in that some simplicity is lost on the user-side...

let s = uname.sysname().unwrap_or_else(|os_s| os_s.to_string_lossy())

would become

// DRY-est (`uname.sysname()` is not repeated)
let s = match uname.sysname().to_os_string().into_string() {
    Ok(s) => s,
    Err(os_s) => String::from(os_s.to_string_lossy()),
};

or

// not DRY!
let t = match uname.sysname().to_str() {
    Some(str) => Cow::from(str),
    None => uname.sysname().to_string_lossy(),
};
// not DRY!
let u = match uname.sysname().to_str().ok_or_else(|| uname.sysname().to_string_lossy()) {
    Ok(str) => Cow::from(str),
    Err(s) => s,
};

But most will just be using uname.sysname().to_string_lossy(). And we have to store the component OsString's, but that seems to be a minimal overhead.

I'll revise the API to return &OsStr's.

I've got a fully functional version up and testing correctly now. I still need to do so work on the unsafe uses to make them "safe", but I'm close to a submission for you both to peruse (It's a 90% re-write).

Thanks for the input.