serenity icon indicating copy to clipboard operation
serenity copied to clipboard

Kernel/Net: SO_ERROR reporting for failed O_NONBLOCK socket connections

Open pdelagrave opened this issue 1 year ago • 4 comments

When using getsockopt with SO_ERROR to get the state of a connecting non-blocking socket, the result always stayed EINPROGRESS no matter if the connection got explicitly refused (RST) or timed out (too many retransmits).

The removed TCPSocket::m_error code was an abstraction solely used to set SO_ERRORs for blocking socket connect attempts. The SO_ERROR assigning logic was moved upstream and now works for both blocking and non-blocking sockets.

Socket::set_so_error() moved from protected to public which didn't feel good but it does seems like NetworkTask::handle_tcp() has the right context and is the right place to make that call.

pdelagrave avatar Jun 05 '23 17:06 pdelagrave

Welcome! :^)

A test is failing, so you might want to make sure everything is in working order first. Additionally, instead of making set_so_error public, you can mark specific classes as friends to allow them to access private/protected methods.

gmta avatar Jun 05 '23 22:06 gmta

Didn't think to run the tests locally first, it should be fixed now.

I'm new to C++ and didn't know about friend yet, thanks for proposing to use it. Although in this specific situation I couldn't make it work as the function to declare as a friend (handle_tcp) is static to the NetworkTask.cpp file. After reading and trying a bunch of different things, I still couldn't come up with a way to make it work. If making Socket::set_so_error() public isn't acceptable or I just missed how to use friend for the function, suggestions would be appreciated.

pdelagrave avatar Jun 06 '23 14:06 pdelagrave

Ah, you wrote NetworkTask::handle_tcp() so I naively assumed that it was part of NetworkTask :-) in that case this might be a fine solution for now. I'd like a second opinion on the new error handling though (and losing some more specific errors that we got from handle_tcp), so I'll leave this open to review.

gmta avatar Jun 06 '23 22:06 gmta

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

stale[bot] avatar Jun 28 '23 01:06 stale[bot]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

stale[bot] avatar Jul 21 '23 00:07 stale[bot]

This pull request has been closed because it has not had recent activity. Feel free to re-open if you wish to still contribute these changes. Thank you for your contributions!

stale[bot] avatar Jul 28 '23 21:07 stale[bot]