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

Add id to UserSocket. #1149

Merged
merged 12 commits into from
Mar 7, 2023
Merged

Conversation

meowjesty
Copy link
Member

Although this does not solve the issue, it's considered a welcome refactor anyway, and will (hopefully) help with #1123 , which is the bigger culprit that needs fixing.

Feedback request

I've left a few warnings in docs saying that you should not use the SOCKET_ALLOCATOR directly, would this be worth it putting into a new module (with the SocketId type) to prevent accidental usage?

@t4lz t4lz self-assigned this Mar 3, 2023
Copy link
Member

@t4lz t4lz left a comment

Choose a reason for hiding this comment

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

I do not really understand what this change is supposed to achieve or what's the improvement, but I left some comments under the assumption this change is needed.

As a general thought about our socket mechanism:
If what we want at the end is to store and access a UserSocket's connection queue by the actual socket and not by its file descriptor (because different file descriptors could be duplicate descriptors of the same socket) couldn't we maybe just keep the connection queue as a field of the UserSocket struct? The duplicate descriptors already point to the same UserSocket via an Arc.

About this:

I've left a few warnings in docs saying that you should not use the SOCKET_ALLOCATOR directly, would this be worth it putting into a new module (with the SocketId type) to prevent accidental usage?

In my opinion it is worth it to pack it an a module. I think it's a very cheap price to pay (or maybe even just a positive code organization feature) for letting the compiler enforce this for us. I recommend using a module, deleting the warnings, and just having a little 1 line comment above the module explaining it's for preventing direct construction.

mirrord/layer/src/socket.rs Outdated Show resolved Hide resolved
mirrord/layer/src/socket.rs Outdated Show resolved Hide resolved
mirrord/layer/src/socket.rs Outdated Show resolved Hide resolved
mirrord/layer/src/socket.rs Outdated Show resolved Hide resolved
mirrord/layer/src/socket.rs Outdated Show resolved Hide resolved
@meowjesty meowjesty requested a review from t4lz March 3, 2023 18:49
mirrord/layer/src/socket.rs Outdated Show resolved Hide resolved
Co-authored-by: t4lz <[email protected]>
Copy link
Member

@t4lz t4lz left a comment

Choose a reason for hiding this comment

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

This can be merged now :)
I still do not completely understand the big plan, but given that it was decided that we need a SocketId, this is good.

This is what I would suggest instead of a SocketId:

If what we want at the end is to store and access a UserSocket's connection queue by the actual socket and not by its file descriptor (because different file descriptors could be duplicate descriptors of the same socket) couldn't we maybe just keep the connection queue as a field of the UserSocket struct? The duplicate descriptors already point to the same UserSocket via an Arc.

@meowjesty meowjesty enabled auto-merge March 6, 2023 14:48
@meowjesty meowjesty disabled auto-merge March 6, 2023 23:58
@meowjesty meowjesty enabled auto-merge March 6, 2023 23:59
@meowjesty meowjesty added this pull request to the merge queue Mar 7, 2023
Merged via the queue into metalbear-co:main with commit e24aebc Mar 7, 2023
@meowjesty meowjesty deleted the issue/1054/socket-id-2 branch March 7, 2023 04:51
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