Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
unix: add Cstruct_unix.{read,write,writev,send,recv} #302
base: main
Are you sure you want to change the base?
unix: add Cstruct_unix.{read,write,writev,send,recv} #302
Changes from 4 commits
99f6a02
87382a1
6002721
2390134
f70b3ac
7d77f0a
04e45e2
3e6d7e4
284960a
ec807a7
fa5ffe7
2cf81f3
4a04478
44ec8fc
803ba0c
aeca686
5920235
471ca03
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor bikeshedding, would move
buf
to the beginning of the block, declarations after statement is not valid C89 (not sure if anyone cares these days)Also void * arithmetic is technically undefined behaviour (also not sure if anyone cares).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
void *
is used here to be in-sync with whatrecv
expects:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack but passing pointer of any type to something that expects a void pointer is ok, my only bit was that we're doing (void *)++, which is undefined behavior (but every compiler implicitly assumes char *) so it's ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://stackoverflow.com/questions/3922958/void-arithmetic says it's a GCC extension, and attempting this should produce warnings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure a
recv
with null flags is very useful, it becomes just read(2), (don't know about windows though).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I’ve copied the flags code from your stubs :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do find these Unix_errors are marginally more useful if you can grep for the function they came from
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as recv
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mildly dislike having to do these potentially O(n) list operations for a writev, and mandate that in the external interface. In your vpnkit usecase, would you be able to pass in a
Cstruct.t array
instead for writev, and then just call writev with simple O(1) time slices instead?It affects the interface we expose in the mli, hence my asking now (as opposed to optimising later)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you expand to a tuple here because the Cstruct.t record type might change? Given this is in the same repository, it's probably safe enough to just pass the Cstruct record in directly to the FFI and save some allocations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that was it -- I was using these stubs in another repo. I'll drop the tuple since we're here.