-
Notifications
You must be signed in to change notification settings - Fork 581
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 readv/writev for illumos #64
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
This PR (HEAD: 0c1c77c) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/sys/+/224238 to see it. Tip: You can toggle comments from me using the |
Message from Gobot Gobot: Patch Set 1: Congratulations on opening your first change. Thank you for your contribution! Next steps: Most changes in the Go project go through a few rounds of revision. This can be During May-July and Nov-Jan the Go project is in a code freeze, during which Please don’t reply on this GitHub thread. Visit golang.org/cl/224238. |
Message from Tobias Klauser: Patch Set 2: Run-TryBot+1 TRY=illumos, solaris Please don’t reply on this GitHub thread. Visit golang.org/cl/224238. |
Message from Gobot Gobot: Patch Set 2: SlowBots beginning. Status page: https://farmer.golang.org/try?commit=b828471c Please don’t reply on this GitHub thread. Visit golang.org/cl/224238. |
Message from Gobot Gobot: Patch Set 2: Build is still in progress... Other builds still in progress; subsequent failure notices suppressed until final report. Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed. Please don’t reply on this GitHub thread. Visit golang.org/cl/224238. |
Message from Gobot Gobot: Patch Set 2: TryBot-Result-1 3 of 16 SlowBots failed: Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.Extra slowbot builds that ran:
Please don’t reply on this GitHub thread. Visit golang.org/cl/224238. |
Message from Tobias Klauser: Patch Set 2:
Please have a look at the TryBot failures. Will this also work on solaris? If not, I think these wrappers should be added to a newly created syscall_illumos.go and we would need to add the code generation for illumos. Please don’t reply on this GitHub thread. Visit golang.org/cl/224238. |
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
This PR (HEAD: a55eb83) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/sys/+/224238 to see it. Tip: You can toggle comments from me using the |
@googlebot I fixed it.. |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
Message from Araragi Hokuto: Patch Set 3:
Forgot to test build after adding preadv/pwritev, sorry about that. Should be fine by now. I haven't test them on solaris 11 yet, but preadv/pwritev does not seem to present on solaris (https://docs.oracle.com/cd/E36784_01/html/E36872/readv-2.html doesn't have entry for them), so I separated those wrapper into syscall_illumos.go for now. However I am not sure about how to add code generation for illumos: Should we copy the whole content of syscall_solaris.go into syscall_illumos.go, or should syscall_illumos.go be some kind of "patch" as for syscall_solaris.go (that is, only for syscalls that do not present on solaris)? Copying the whole content seems to be redundant, as illumos have all the syscalls current present in syscall_solaris.go (i assume?). But if we add only necessary content into syscall_illumos.go, then all the code generation needed on solaris will also be required on illumos. How should we modify mkall.sh if that is the case? BTW: Currently I generated zsyscall_illumos_amd64.go with mksyscall_solaris.go, but there's an error about "syscall" being imported and not used. I modified the output by hand for now, but should we add a mksyscall_illumos.go? (This also suffers from the problem above) Please don’t reply on this GitHub thread. Visit golang.org/cl/224238. |
Message from Tobias Klauser: Patch Set 3:
Since GOOS=illumos implies GOOS=solaris, I'd suggest the latter approach: adding syscall_illumos.go with just the additional functionality with illumos has, but solaris doesn't.
Exactly.
We could do it in a similar way as the BSDs and darwin which also use syscall_bsd.go as a base line and then also have syscall_$GOOS.go on top.
I think we can just modify mksyscall_solaris (maybe with an additional -illumos flag) to generate code accordingly so it doesn't need manual patching. Please don’t reply on this GitHub thread. Visit golang.org/cl/224238. |
This PR (HEAD: 45d02f9) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/sys/+/224238 to see it. Tip: You can toggle comments from me using the |
Message from Ian Lance Taylor: Patch Set 4: Run-TryBot+1 Please don’t reply on this GitHub thread. Visit golang.org/cl/224238. |
Message from Gobot Gobot: Patch Set 4: SlowBots beginning. Status page: https://farmer.golang.org/try?commit=61a1cc2e Please don’t reply on this GitHub thread. Visit golang.org/cl/224238. |
Message from Gobot Gobot: Patch Set 4: TryBot-Result+1 SlowBots are happy.
Please don’t reply on this GitHub thread. Visit golang.org/cl/224238. |
Message from Araragi Hokuto: Patch Set 4:
I have added -illumos flag for mksyscall_solaris (which, when present, will omit the syscall import statement in output), and modified mkall.sh so it will process both solaris and illumos files. However, I think it's worth mentioning that in my OmniOS setup, the output of mkerrors.sh and "cgo -godefs" is slightly different from files already present. the output of mkerrors have both added and removed lines, and the output of cgo have some structure name not transformed "like _Ctype_struct_timespec" in "Stat_t" which cause it fail to compile. I left them out for now since I am not sure what to do and I feel it is beyond the scope of this patch, but maybe illumos require some more specific handling in those respects? Please don’t reply on this GitHub thread. Visit golang.org/cl/224238. |
Message from Ian Lance Taylor: Patch Set 4:
That is probably https://golang.org/issue/37622. Please don’t reply on this GitHub thread. Visit golang.org/cl/224238. |
Message from Tobias Klauser: Patch Set 4: (2 comments) Please don’t reply on this GitHub thread. Visit golang.org/cl/224238. |
Message from Araragi Hokuto: Patch Set 4:
Looks like it, so I'll just leave ztypes problem for now. What should we do about zerrors_solaris_amd64.go? I looked at the diff of two version and found out that quite a lot of constants got dropped on illumos. I'm afraid that will cause problem on solaris. Here is the diff: https://gist.github.com/AraragiHokuto/1f4d42de37334d16aa0745975f58e85e Please don’t reply on this GitHub thread. Visit golang.org/cl/224238. |
This PR (HEAD: abfeaab) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/sys/+/224238 to see it. Tip: You can toggle comments from me using the |
This PR (HEAD: 841e13d) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/sys/+/224238 to see it. Tip: You can toggle comments from me using the |
Message from Araragi Hokuto: Patch Set 6: So that's what I've came up with after one night: I've modified ./mkerrors.sh to have it set build tags on solaris with an addition !illumos, and generated a new zerrors_illumos_amd64.go. This makes illumos can have it own zerrors file while leave defs in zerrors_solaris_amd64.go untouched. Do you think this is a reasonable workaround, or there's a better way to solve this? I'm quite new to go internals so please let me know how we normally handle such situation. Please don’t reply on this GitHub thread. Visit golang.org/cl/224238. |
Message from Tobias Klauser: Patch Set 6:
Are the error consts needed for the functionality of this CL, i.e. for readv/writev to work? If not, I'd suggest to create a follow-up CL/PR with these changes. Please don’t reply on this GitHub thread. Visit golang.org/cl/224238. |
Message from Araragi Hokuto: Patch Set 6:
man page of readv/writev state that their possible errno values is nothing more than those common POSIX errnos, so I don't think it's necessary for this CL. Therefore I guess we could leave mkall.sh as in patchset 5 for now? I'm new to Gerrit, so I'm not sure what's the limit of this system. Should I revert patchset 6 (the ./mkerrors.sh change) to patchset 5 or it's better just leave it as it so commit history won't be messed up? Thank you. Please don’t reply on this GitHub thread. Visit golang.org/cl/224238. |
Message from Tobias Klauser: Patch Set 6:
Ok, so let's leave the error changes out of this CL for now. Feel free to send additional CLs for zerrors and ztypes updates.
It seems you're using GitHub, so you can just modify your existing commit/PR and force push. Gerrit will pick it up and squash all commits in the PR into a single commit for the CL. See https://golang.org/wiki/GerritBot for details. Please don’t reply on this GitHub thread. Visit golang.org/cl/224238. |
[p]readv/[p]writev system calls are present on illumos, yet wrapper for them was not added. This commit adds those system calls on illumos. Since preadv/pwritev doesn't seem to be present on solaris, this commit also updates mkall.sh to generate system call wrapper specific for illumos.
This PR (HEAD: 81a479f) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/sys/+/224238 to see it. Tip: You can toggle comments from me using the |
Message from Araragi Hokuto: Patch Set 7:
Reverted to patchset 5 and force pushed (to clean up commit history). Thank you for your patience. Please don’t reply on this GitHub thread. Visit golang.org/cl/224238. |
Message from Tobias Klauser: Patch Set 7: Run-TryBot+1 TRY=illumos, solaris Please don’t reply on this GitHub thread. Visit golang.org/cl/224238. |
Message from Gobot Gobot: Patch Set 7: SlowBots beginning. Status page: https://farmer.golang.org/try?commit=2db4df9a Please don’t reply on this GitHub thread. Visit golang.org/cl/224238. |
Message from Gobot Gobot: Patch Set 7: TryBot-Result+1 SlowBots are happy.
Please don’t reply on this GitHub thread. Visit golang.org/cl/224238. |
iovec read/writes (readv, writev, preadv, pwritev) are present on illumos, but they weren't added to the package. This commit adds those missing features. Updates golang/go#36201 Change-Id: Iecf2bab7ef846f5ca5d693e833491d819618c15d GitHub-Last-Rev: 81a479f GitHub-Pull-Request: #64 Reviewed-on: https://go-review.googlesource.com/c/sys/+/224238 Run-TryBot: Tobias Klauser <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Tobias Klauser <[email protected]>
Message from Tobias Klauser: Patch Set 7: Code-Review+2 Thanks. Please don’t reply on this GitHub thread. Visit golang.org/cl/224238. |
This PR is being closed because golang.org/cl/224238 has been merged. |
iovec read/writes (readv, writev, preadv, pwritev) are present on
illumos, but they weren't added to the package. This commit adds
those missing features.
Updates golang/go#36201