Skip to content
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

Windows: sa_family_t should use ADDRESS_FAMILY from windows-sys #462

Closed
nerditation opened this issue Aug 25, 2023 · 5 comments · Fixed by #463
Closed

Windows: sa_family_t should use ADDRESS_FAMILY from windows-sys #462

nerditation opened this issue Aug 25, 2023 · 5 comments · Fixed by #463

Comments

@nerditation
Copy link
Contributor

in #414 socklen_t and sa_family_t were changed to use types from windows-sys, which themselves were added to the win32metadata by request, in microsoft/win32metadata#1041 (both as NativeTypedefs). however, sa_family_t was later changed to MetadataTypedef in microsoft/win32metadata#1538 because it was an "invented" handle type (meaning it doesn't exist in any of the Windows SDK header files but only in win32metadata). socklen_t, on the other hand, does exists in the Windows SDK header "WS2tcpip.h", so it remains NativeTypedef.

it turns out that the equivalent to POSIX sa_family_t in Winsock2 is the ADDRESS_FAMILY type, and it existed forever. I think it's a mistake to request a win32metadata only typedef while ingoring the proper Windows SDK type,- and I'd suggest we change our sa_family_t to use the proper type, i.e. windows_sys::Win32::Networking::Winsock::ADDRESS_FAMILY, which also makes it consistent with other Winsock2 APIs, for instance, in windows-sys (and also windows), the type of SOCKADDR::sa_family and SOCKADDR_STORAGE::sa_family is ADDRESS_FAMILY, the constants AF_INET, AF_INET6 are defined using type ADDRESS_FAMILY, and some iphelper functions like GetIpForwardTable2() accept ADDRESS_FAMILY as argument.

example code in the `windows` crate where `ADDRESS_FAMILY` is used

SOCKADDR

https://github.com/microsoft/windows-rs/blob/891c469706d314fcf0e3a7913f45970ce11b13f0/crates/libs/windows/src/Windows/Win32/Networking/WinSock/mod.rs#L13218-L13223

SOCKADDR_IN::sin_family

https://github.com/microsoft/windows-rs/blob/891c469706d314fcf0e3a7913f45970ce11b13f0/crates/libs/windows/src/Windows/Win32/Networking/WinSock/mod.rs#L13314-L13321

AF_INET and AF_INET6

https://github.com/microsoft/windows-rs/blob/891c469706d314fcf0e3a7913f45970ce11b13f0/crates/libs/windows/src/Windows/Win32/Networking/WinSock/mod.rs#L1782-L1785

btw, I discovered #414 when I was investigating the discrepancy of wrapper types from the windows crate. microsoft/windows-rs#2627

@Thomasdezeeuw
Copy link
Collaborator

That seems reasonable.

One possible difficulty is that SockAddr::family returns sa_family_t exported by the sys module, currently that is windows_sys::Win32::Networking::WinSock::sa_family_t for Windows. It could be a breaking change to change it, even though it's all the same primitive type underneath.

@nerditation
Copy link
Contributor Author

It could be a breaking change to change it, even though it's all the same primitive type underneath.

it's a valid concern, but I think changing from sa_family_t to ADDRESS_FAMILY is no different than changing from u16 to sa_family_t as done in #414

what's more, how is SockAddr::family() supposed to be used by the caller? they compare the return value to AF_INET, AF_INET6, right? in windows-sys these constants are defined as:

pub const AF_INET: ADDRESS_FAMILY = 2u16;
pub const AF_INET6: ADDRESS_FAMILY = 23u16;

so in this regard, the caller code like this

let family = sockaddr.family();
if family == AF_INET {
    todo!()
}

is technically already "broken" if we keep it as the current definition, and change the return type to ADDRESS_FAMILY is no more "broken" than it's current form (but actually more "correct").

@Thomasdezeeuw
Copy link
Collaborator

it's a valid concern, but I think changing from sa_family_t to ADDRESS_FAMILY is no different than changing from u16 to sa_family_t as done in #414

Fair point.

what's more, how is SockAddr::family() supposed to be used by the caller? they compare the return value to AF_INET, AF_INET6, right? in windows-sys these constants are defined as:

pub const AF_INET: ADDRESS_FAMILY = 2u16;
pub const AF_INET6: ADDRESS_FAMILY = 23u16;

To be honest, I've only done this on Unix, so I don't have a good answer to this.

so in this regard, the caller code like this

let family = sockaddr.family();
if family == AF_INET {
    todo!()
}

is technically already "broken" if we keep it as the current definition, and change the return type to ADDRESS_FAMILY is no more "broken" than it's current form (but actually more "correct").

Yeah, fair enough. Do you want to send a pr to change the type?

@nerditation
Copy link
Contributor Author

Do you want to send a pr to change the type?

sure, #463 is created

@Thomasdezeeuw
Copy link
Collaborator

This was done in #463.

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 a pull request may close this issue.

2 participants