Skip to content

Implement bits of wininet #5

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sollyucko
Copy link

I wanted to implement simple HTTP requests without requiring a monolithic networking library, so I figured I might as well add the API code here too.

Copy link
Owner

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I'm in principle fine with this addition, but I think at least two things need to happen:

  • The dependency on widestring needs to be removed.
  • Every public API item needs to be documented. This only passed CI because I didn't put #![deny(missing_docs)] in the crate root, which I usually do. Please look at the docs on other APIs for an idea of what I would expect here.

There are a few problems with the use of widestring here. The most serious one is that this PR makes it a public dependency of this crate, which seems like a bad idea to me, especially when widestring has APIs for easily converting between its string types and &[u16].

Another problem is that I'm generally dependency averse, and with something like winapi-util being used in lots of places, adding a new dependency at this level needs to be exceptionally well justified. I'm not sure that widestring is pulling its weight.

Finally, winapi-util is really supposed to provide convenience layers for Windows APIs. How important is it that we expose APIs that work with UTF-16 directly instead of using &str? At the very least, I think the main APIs should be using &str, and if necessary, we can provide separate APIs that take a raw &[u16].

@sollyucko
Copy link
Author

Finally, winapi-util is really supposed to provide convenience layers for Windows APIs. How important is it that we expose APIs that work with UTF-16 directly instead of using &str? At the very least, I think the main APIs should be using &str, and if necessary, we can provide separate APIs that take a raw &[u16].

Should I use str::encode_utf16 (or possibly OsStrExt::encode_wide) and collect it into a new Vec each time? (s.encode_utf16().chain(once(0)).collect::<Vec<_>>())

especially when widestring has APIs for easily converting between its string types and &[u16]

Is the issue here that it would allow safe code to pass invalid WTF-16 to the functions? (What would an example of invalid WTF-16 be? Encoding a non-existent character such as U+FFFE?)


Every public API item needs to be documented. This only passed CI because I didn't put #![deny(missing_docs)] in the crate root, which I usually do. Please look at the docs on other APIs for an idea of what I would expect here.

Would this be good for HInternet::open, for example?

Create an owned handle for performing network requests.

This corresponds to calling InternetOpenW.


A few other things:

  • Should I change the Options into io::Results using io::Error::last_os_error()? (Or use InternetGetLastResponseInfoW in the case of InternetConnectW?)
  • Should I mark HConnect::open_request unsafe due to the use of a raw pointer? Do you have any ideas for avoiding that?
  • Both HConnect::open_request and HRequest::send cast *const to *mut under the assumption that the lack of constness is accidental. Is that okay?

@BurntSushi
Copy link
Owner

BurntSushi commented Nov 11, 2020

Should I use str::encode_utf16 (or possibly OsStrExt::encode_wide) and collect it into a new Vec each time? (s.encode_utf16().chain(once(0)).collect::<Vec<_>>())

For &str APIs, yes, absolutely. This is exactly what std does.

I think the OsStr API is probably not relevant here, unless we provide an OsStr API. That might actually be nicer. That is, we could accept an AsRef<OsStr> instead of a &str directly, and then use OsStrExt::encode_wide. That would still permit passing &str.

Is the issue here that it would allow safe code to pass invalid WTF-16 to the functions? (What would an example of invalid WTF-16 be? Encoding a non-existent character such as U+FFFE?)

I don't think WTF-16 is a thing. I've only ever heard of WTF-8, which is what OsStr uses. The point I was trying to make is that I don't see any good reason for putting widestring types into the public API of widestring. (And separately, I'm not sure we really need widestring at all, even internally.)

Would this be good for HInternet::open, for example?

Create an owned handle for performing network requests.
This corresponds to calling InternetOpenW.

Yes, I think that's okay. If there are any other particulars about the API specifically exposed by this crate, then those should be noted too. (I'm not saying there are, just communicating the general principle.)

Oh, I missed that. But yes, of course. If there is an error to report then we absolutely should be exposing that!

  • Should I mark HConnect::open_request unsafe due to the use of a raw pointer? Do you have any ideas for avoiding that?

I missed this as well. The problem is, I don't really have the time to dig into these Windows APIs and do the design work for them. Can you accept a higher level type and maybe convert that into a *const *const u16? Maybe a Vec<String> or &[String] or &[S] where S: AsRef<str>? If *const *const u16 is the only option, then we should carefully document it. And yes, that would unfortunately require marking the routine as unsafe since a caller could pass an input that leads to UB.

  • Both HConnect::open_request and HRequest::send cast *const to *mut under the assumption that the lack of constness is accidental. Is that okay?

Yes. AIUI the only real difference between these types is variance. In general, it's okay to cast between them because when it comes to raw pointers, Rust doesn't make any aliasing assumptions (unlike with & and &mut references). This SO answer seems to agree with my understanding: https://stackoverflow.com/questions/55664870/what-are-the-differences-between-const-t-and-mut-t-raw-pointers

And see also: https://internals.rust-lang.org/t/what-is-the-real-difference-between-const-t-and-mut-t-raw-pointers/6127/2

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