Bugfix/ipv6 handling#216
Conversation
Merging from the upstream
…inet or :inet6 based on peer's socket address to the client's socket
There was a problem hiding this comment.
Hi. Thanks for your contribution! Please run GitHub Actions manually in your fork and provide here the link to success CI.
How to:
Open https://github.com/se-apc/eradius
Select tab Actions - https://github.com/se-apc/eradius/actions
Select tab CI
Find button Run workflow
Select in Run workflow branch master and click Run workflow
vkatsuba
left a comment
vkatsuba
left a comment
There was a problem hiding this comment.
Please keep one code style - use 4 spaces instead of 2.
vkatsuba
left a comment
vkatsuba
left a comment
There was a problem hiding this comment.
If you can, please provide test cases.
| -spec send_request(nas_address(), #radius_request{}, options()) -> | ||
| {ok, binary(), eradius_lib:authenticator()} | {error, 'timeout' | 'socket_down'}. | ||
| send_request({Host, Port, Secret}, Request, Options) | ||
| send_request({Host, Port, Secret}, Request, Options) |
There was a problem hiding this comment.
please, don't mix pure formatting and white space changes with functional changes.
If you think the whitespace problems need fixing, create a separate pull request for that.
Sure thing. I shall need to revisit some tests that have already been present due to the APIs changes. The functionality itself I had tested manually. I am not an Erlang major (not main main programming language) so perhaps a joint effort in the expansion of the available test suite would be well appreciated.
|
QQ on this one. I shall have been probably merging the current master branch into our fork in any way so another merge commit shall appear. Would the commits' squashing into one do? I am asking because such a synchronisation with the upstream master can be advantageous on many levels. OOC what would be the reasons behind your disliking that? |
Amending the test suite to fit the change to the eradius_client_socket,start/3 to start/4 API
| end. | ||
|
|
||
| %% @private | ||
| inet_family_based_on_peer(_PeerSocket = {{_, _, _, _}, _port}) -> |
There was a problem hiding this comment.
Only inet_family_based_on_peer/1 is called everywhere. So the:
inet_family_based_on_peer(_PeerSocket) ->
[].
should be removed at all and inet_family_based_on_peer/2 should be reworked to inet_family_based_on_peer/1 like:
inet_family_based_on_peer({_, _, _, _}) ->
[inet];
inet_family_based_on_peer({_, _, _, _, _, _, _, _}) ->
[inet6].
That should be safe to do as inet_family_based_on_peer/1 will be called with proper IP address all the time.
| IP = lists:nth(Index, IPs), | ||
| send_request({IP, Port, Secret}, Request, Options); | ||
| _ -> error(badarg) | ||
| _Err -> |
There was a problem hiding this comment.
This could be simplified a bit. The whole send_request function may look like this:
send_request({Host, Port, Secret}, Request, Options)
when ?GOOD_CMD(Request) andalso is_list(Host) ->
case gethostbyname(Host) of
{ok, #hostent{h_addr_list = [IP]}} ->
send_request({IP, Port, Secret}, Request, Options);
{ok, #hostent{h_addr_list = [_ | _] = IPs}} ->
Index = rand:uniform(length(IPs)),
IP = lists:nth(Index, IPs),
send_request({IP, Port, Secret}, Request, Options);
_Err -> error(badarg)
end
end;
gethostbyname(Host) ->
case inet:gеthostbyname(Host, inet6) do
{error, _} ->
inet:gethostbyname(Host, inet);
Hostent ->
Hostent
end.
|
Hello @metelik. First of all thanks for PR. I've leaved some comments to simpilfy a couple of places. Could you please adjust the PR. |
Sure thing @0xAX . I shall have been looking into it. I am occupied with another project (for which I am being paid for), hence resolving all the requested changes may drag tiny bit :/ |
@metelik if you don't have a lot of time to update it, we can do it in our side with saving git commits history with your changes. What do you think about it? |
|
|
Moved to #234 |
4 participants
Changes: