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

["John Plevyak (JIRA)" <ji...@apache.org>]

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

John Plevyak commented on TS-320:
---------------------------------

Reviewed, looks good, here are a few comments

- This patch fixes a potential problem with EDGE_TRIGGERED on FreeBSD/Solaris when an error occurs
      whereby the events might not be cleared before the close().
    * FreeBSD and Solaris queue the kqueue/poll() events in user space and only flush them
      at the poll, so it is best not to start() then close() as the event will still be in the queue and
      could be applied to a different connection with the same socket #
- I would remove #if 0 in connectUp and the dead code above it
- Do we want to try the safe_setsockopt in Connection::open if IP_TRANSPARENT
  is not defined ?  Is it possible for there to be a conflict on # 19 ?
- Should move the cleaner from UnixConnection.cc into a more general place
  (libinktomi++ ?)
- is_connected and is_bound in Connection look like they are old debugging cruft,
  we might want to get rid of them.

None of this needs to be fixed before checkin since it is existing.
I would encourage checkin sooner rather than later because of the EDGE_TRIGGER issue.

I assume that now you have the perms you want to checkin?  Otherwise, tell me and I can.

> 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.1.0
>            Reporter: Alan M. Carroll
>            Assignee: Alan M. Carroll
>            Priority: Minor
>             Fix For: 2.1.2
>
>         Attachments: ts-320-diff-6.txt
>
>   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.