[jira] Commented: (TS-320) Do some cleanup on Connection::fast_connect and Connection::bind_connect

["Alan M. Carroll (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/TS-320?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12873764#action_12873764 ] 

Alan M. Carroll commented on TS-320:
------------------------------------

As usual, the plan did not survive contact with the codebase.

In the end, the low level connect calls were invoked only from one place, UnixNetVConnection::connectUp. Therefore the result was tailored to the needs of that call site. Connection::connect, Connection::fast_connect, and Connection::bind_connect were replaced by two new methods.

open: Create the socket and set all options that are set before connection.
connect: Connect the socket to the target. Set options that must be set after connection.

These are only somewhat similar to the original methods. All arguments other than the remote IP address and port have been moved in to the NetVCOptions structure. The default values for this are controlled solely by the NetVCOptions::reset method. All can be overridden, none of them are hardwired or "re-defaulted" in the Connection methods.

For the ICP code, that at last went as orginal intended. It continued to be the case that as far as I can tell the Connection object is used only to store a sockaddr_in. It's not completely clear because the ICP code wasn't updated when the socket argument was added to Connection::bind_connect. Therefore I made a method to set the internal sockaddr_in and had that called from the ICP code instead of Connection::bind_connect.


> Do some cleanup on Connection::fast_connect and Connection::bind_connect
> ------------------------------------------------------------------------
>
>                 Key: TS-320
>                 URL: https://issues.apache.org/jira/browse/TS-320
>             Project: Traffic Server
>          Issue Type: Improvement
>          Components: Cleanup
>    Affects Versions: 2.0.0a
>            Reporter: Alan M. Carroll
>            Assignee: Alan M. Carroll
>            Priority: Minor
>   Original Estimate: 168h
>  Remaining Estimate: 168h
>
> Action Plan:
> Unify bind_connect, fast_connect.
> 1. Split out the binding of the target address in to a new method
> Connection::bind_target(target_ip, target_port);
> This simply copies to the target IP address and port to Connection::sa. As far as I can tell this is the only thing the calls to bind_connection from IPC.cc do. We can then change those calls to direct calls to bind_target. Then we should be able to eliminate all the bool flags on bind_connect which aren't used elsewhere.
> 2. Remove the my_ip argument from bind_connect. Use spoof_ip in NetVCOptions instead. At this point the signatures for fast_connect and bind_connect are now identical.
> 3. Add support for the IP_TRANSPARENT sock opt provided by TPROXY in to NetVCOptions, either as a separate flag or (better) merged in to NetVCOptions::sockopt_flags. This would mean that spoof_ip is treated as a local address (i.e., an address already assigned to an interface on the local system) if not set, or as an arbitrary address requiring TPROXY support if set. If we conditionally define the mask as IP_TRANSPARENT or 0 depending on TPROXY availability we should be able to limit conditional compilation to that location (as the check will always fail if IP_TRANSPARENT is not available).
> 4. Move SO_REUSEADDR to NetVCOptions::sockopt_flags. This is set in bind_connect but not fast_connect.
> 5. Put an TCP vs. UDP flag in NetVCOptions. This would follow from the code, but as far as I can tell this is never used. It is passed in from the ICP logic but, because that also sets the "no bind" flag, it doesn't get invoked. It may be that bind_connect should just be implicitly TCP.
> 6. Verify that NetVCOptions default constructed values are such that bind_connect with a default constructed NetVCOptions does the same thing that fast_connect does.
> 7. Add an option (in the signature or in NetVCOptions) to handle the epoll support. AFAICT this is done in all cases where either fast_connect or bind_connect is called other than IPC (which we have already removed) so we could hard wire it. Comments in the code indicated this is already considered a problem.
> After all of this, we can replace all invocations of fast_connect with bind_connect with no change in functionality.
> Note: This is in preparation for working on TS-291.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.