Use windows-sys 0.59.0.

[qrnch-jan]

Upgrade to windows-sys 0.59.

Biggest fallout that was fixed:

• Earlier windows-sys crate defined HANDLE as a usize. However, technically it's a void*, which is what later windows-sys crates use instead. Because c_void is !Send this made it impossible to pass the HANDLE into a the tokio thread pool. To work around this, a Send:able newtype was introduced for the sole purpose of passing the event HANDLE over the thread boundary. Not pretty, but completely hidden from user.

Other:

• windows-sys used to have some weird inconsistencies where well-known 16-bit constants where made 32-bit, but not all related interfaces were changed to match. All these seem to have been fixed to proper 16-bit values.
• Some tests have constants that rely on HANDLE being usize. These just naively cast to *mut std::ffi::c_void now.

[stappersg]

[Wojtek242]

@qrnch-jan thanks for the PR! I don't understand the semver check error at the moment, but I'll have a look at some point (might be in a few weeks due to Christmas). Maybe it makes sense to you.

[Wojtek242]

@stappersg I appreciate your interest in this project. You have left several comments on the latest pull requests of a contributor to this project. In what way do you feel that your remarks help make the contributions better?

[qrnch-jan]

@qrnch-jan thanks for the PR! I don't understand the semver check error at the moment, but I'll have a look at some point (might be in a few weeks due to Christmas). Maybe it makes sense to you.

TIL about cargo semver-checks -- neat!

This was a further fallout from HANDLE becoming a pointer in the updated windows-sys crate, because it caused PacketStream to become !Send.

[stappersg]

On Tue, Dec 17, 2024 at 12:57:04PM -0800, Wojciech Kozlowski wrote: In what way do you feel that your remarks help make the contributions better?

That git can be used as the nice version control system it is. Getting the git repository ready for `git bisect`, `git blame` and just `git log`. Groeten Geert Stappers -- Silence is hard to parse

[Wojtek242]

Thanks for fixing it @qrnch-jan! Having looked at your PR, I'm wondering how important is it that you cache the handle? This seems to be the root of all your safety issues. Is there a significant performance gain in avoiding the capture.get_event() call?

Also, is there any user visible change? If so, can you update the CHANGELOG?

[Wojtek242]

@stappersg

On Tue, Dec 17, 2024 at 12:57:04PM -0800, Wojciech Kozlowski wrote: In what way do you feel that your remarks help make the contributions better?
That git can be used as the nice version control system it is. Getting the git repository ready for git bisect, git blame and just git log. Groeten Geert Stappers -- Silence is hard to parse

I think that's a reasonable argument. However, we also need to acknowledge that pcap is a small repository and so the benefit from this is smaller than in a much bigger repository. Furthermore pcap has few contributors. Therefore, I think it is also important to not impose undue burden on them to avoid discouraging future contributions. I would prefer if reviews focused on the code itself and potentially the comments if they are unclear.

[stappersg]

On Thu, Dec 19, 2024 at 10:19:13AM -0800, Wojciech Kozlowski wrote: @stappersg > On Tue, Dec 17, 2024 at 12:57:04PM -0800, Wojciech Kozlowski wrote: > > In what way do you feel that your remarks help make the contributions better? > > > > That git can be used as the nice version control system it is. Getting > the git repository ready for `git bisect`, `git blame` and just > `git log`. I think that's a reasonable argument. However, we also need to acknowledge that pcap is a small repository and so the benefit from this is smaller than in a much bigger repository. Furthermore pcap has few contributors. Therefore, I think it is also important to not impose undue burden on them to avoid discouraging future contributions.

In my words: "Low standard contribution is better as no contribution"

I would prefer if reviews focused on the code itself and potentially the comments if they are unclear.

I give up my preference for medicore contribution (for this project). Groeten Geert Stappers -- Silence is hard to parse

[qrnch-jan]

Thanks for fixing it @qrnch-jan! Having looked at your PR, I'm wondering how important is it that you cache the handle? This seems to be the root of all your safety issues. Is there a significant performance gain in avoiding the capture.get_event() call?

Are you're asking about EventHandle::poll_ready()? If not, then please disregard the next paragraph. :)

The issue is that the handle needs to passed to WaitForSingleObject() inside a spawn_blocking() call. Because we can't send the capture device over thread boundaries there, we pass the handle instead.

Also, is there any user visible change? If so, can you update the CHANGELOG?

I would like to say that there are no user-visible changes. However, there's the issue of the windows-sys HANDLE change. Basically, because we return an instance of HANDLE (returned via Capture::get_event()) one could argue there's a change to the public interface. However, we don't actually reexport the HANDLE type -- that comes from windows-sys. Any application that uses HANDLE in a struct/enum and tried to pass it over thread boundaries will encounter the same issue I did with that it doesn't get auto-traited with Send. But that's a more of a windows-sys issue.

I can make a note about this in the changelog though.

[Wojtek242]

Thanks for fixing it @qrnch-jan! Having looked at your PR, I'm wondering how important is it that you cache the handle? This seems to be the root of all your safety issues. Is there a significant performance gain in avoiding the capture.get_event() call?

Are you're asking about EventHandle::poll_ready()? If not, then please disregard the next paragraph. :)

The issue is that the handle needs to passed to WaitForSingleObject() inside a spawn_blocking() call. Because we can't send the capture device over thread boundaries there, we pass the handle instead.

No, I mean the handle field of EventHandle. It seems to me that the HANDLE object is kept in two places inside PacketStream. Once, in the capture from which you can obtain it with get_event() and once in EventHandle.handle. This suggests to me that you might not need to cache the HANDLE in event_handle. Is it possible to restructure the code such that EventHandle no longer keeps a cached HANDLE and instead calls capture.get_event() whenever it needs it. I think this would reduce the amount of unsafe code and make it safer against future changes. Does this make sense? Do you think this is doable?

Also, is there any user visible change? If so, can you update the CHANGELOG?

I would like to say that there are no user-visible changes. However, there's the issue of the windows-sys HANDLE change. Basically, because we return an instance of HANDLE (returned via Capture::get_event()) one could argue there's a change to the public interface. However, we don't actually reexport the HANDLE type -- that comes from windows-sys. Any application that uses HANDLE in a struct/enum and tried to pass it over thread boundaries will encounter the same issue I did with that it doesn't get auto-traited with Send. But that's a more of a windows-sys issue.

I can make a note about this in the changelog though.

The CHANGELOG looks good to me as it is then at the moment.

[qrnch-jan]

Does this make sense? Do you think this is doable?

I will take a look!