Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/22882 )
Change subject: [socket] Fix test failure caused due to unsupported socket option ...................................................................... Patch Set 1: > Patch Set 1: > > > > Patch Set 1: > > > > > > > Patch Set 1: > > > > > > > > (1 comment) > > > > > > The two tests are consistently failing on my local Ubuntu 20.04 > > machine for debug build (rebase 9e661cb34). > > > > > > It appears that in my case, for some reason, the socket shutdown > > is failing consistently due to which subsequent operation of > > setting SO_REUSEPORT option fails on unix domain socket. If socket > > shutdown completes successfully, setting SO_REUSEPORT option may > > not fail. I will need to validate this theory though, by running > > some tests. > > > > > > Considering above explanation holds, we need to address two > > issues: > > > 1. Ensure proper socket cleanup is done successfully in order to > > avoid reuseport failure. > > > 2. Ensure unix domain sockets are not unknowingly used for > > SO_REUSEPORT socket option and expected to work fine. > > > > > > I ran a sample routine, that sets socket option(SO_REUSEPORT) on a > > AF_UNIX type socket, on different linux kernel versions (5.15.0 and > > 6.15-rc7) and it became clear that kernel versions containing > > commit [1] are gracefully erroring out on non-inet sockets. The > > change was introduced to fix a different issue however. > > > > [1]: > https://github.com/torvalds/linux/commit/5b0af621c3f6ef9261cf6067812f2fd9943acb4b > > > > Socket cleanup issue (mentioned above) turns out to be something > > different and could be related to C++ version installed on my > > ubuntu linux variant. I haven't figured that out yet. > > > > I still believe, we need to make sure we are not unknowingly using > > SO_REUSEPORT option while setting sockops on a AF_UNIX type socket. > > Current change should help in doing that. > > OK, but why to change socket.cc? > > Also, maybe update the description with your findings, since the statement > that SO_REUSEPORT for a Unix domain socket isn't supported isn't true. SO_REUSEPORT macro has been available there since kernel 3.9 and it would be safe to assume that it is available in all the linux distros today, so there is no need to have this macro availability check, hence the change in socket.cc. SO_REUSEPORT for a Unix domain socket is not supported is true because there is no proper handling of such a case in kernel. Also, there is no equivalent of sin_port in sockaddr_un structure. For the kernel versions (with [1] commit), setsockopt fails with appropriate error which is good because we will get to know the error before bind operation. However, if there is a wrong expectation of successfully setting reuseport option on unix domain socket, we can certainly correct that expectation with this check. The check will also handle the cases where [1] is not present in the kernel, potentially leading to undefined behaviour. -- To view, visit http://gerrit.cloudera.org:8080/22882 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21 Gerrit-Change-Number: 22882 Gerrit-PatchSet: 1 Gerrit-Owner: Ashwani Raina <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 21 May 2025 09:14:21 +0000 Gerrit-HasComments: No
