Skip to content

Conversation

@scottatchley
Copy link
Contributor

This does not include any source changes.

This does not include any source changes.
@dillow
Copy link

dillow commented Jul 26, 2011

I mostly approve, a few nits:

  • Please watch out for trailing whitespace issues. 'git diff --color=auto' will highlight those in red for you.
  • There are a number of typos:
    signalled => signaled
    cci_event_connect_req_t => cci_event_connect_request_t (or vice versa)
    cci_event_device_failed => cci_event_device_failed_t
    cci_event_connect_device_failed_t => cci_event_device_failed_t
    etc.

Please fix these and rebase; I'd prefer to not have a long chain of fixups in the history.

@dillow
Copy link

dillow commented Jul 26, 2011

Also, check out the duplicity between cci_event_connect_request_t and cci_conn_req_t; I need the latter for accept/reject, but I don't have a way to get there from the event; perhaps we keep cci_get_conn_req() around, but then there is no link from event to the request.

We didn't want to embed the conn_req_t stuff in the event as we thought people might sit on them for a while -- LDAP auth, etc. So should we then have a pointer in the event that becomes a cookie for accept/reject() to use to get to the driver specific state?

@dillow
Copy link

dillow commented Jul 26, 2011

We could also preallocate 'backlog' conn_req events, and just those as the key; we could copy any data needed out of the RX ring -- I'm assuming it is a maximum of 1 KB. This keeps use from hogging the AM resources, and merges the event.

@scottatchley
Copy link
Contributor Author

Dave, re-whitespace issues, I have have been slowly cleaning up the header file as we touch related code. I set c_space_errors = 1 in vimrc, but I turned it off for the conf call. ;-)

I will fix and rebase the typos. This pull request was rushed. I should have waited.

There is so much overlap with the remove-header pull request, that we may be better off waiting until that one is landed and letting me start over.

@scottatchley
Copy link
Contributor Author

I will close this request until the topic/remove-header pull request is landed to avoid overlap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants