syscall: incorrect value for TCGETS and TCSETS in std syscall pkg on ppc64x #19560
Comments
laboger
changed the title
|
This was found as a result of investigating opencontainers/runc#1364 |
|
@bradfitz Who can answer questions about this issue? Is the answer supposed to be that users should be using golang.org/x/sys/unix instead of syscall from golang std? Personally I think that the syscall package in golang should be correct. But it is not clear to me how to get the correct value generated from the sh and pl files that are used to create them, and why the one in golang.org/x is correct but the other is not. |
|
Why is std incorrect? Have these values changed over time? If so, that suggests even more that the answer is using x/sys/unix and not trying to keep syscall up to date. But yes, the real answer is that everybody should use x/sys and pretend that the std package "syscall" doesn't exist. |
|
I'll slightly disagree with @bradfitz and say that if the values in the syscall package were always wrong, then I think it would be OK to correct them. It looks like they were introduced by @minux in https://golang.org/cl/127170043, where he says they were autogenerated. Minux, do you know anything about these values? |
|
Yeah, if they were always wrong and they by definition never change and it's just a result of a bug in our generator program, I'm fine changing it. But that does mean that users will need to wait for a new version of Go for the fix, whereas if |
|
It looks like this problem was identified here https://groups.google.com/forum/#!msg/golang-nuts/K5NoG8slez0/mixUse17iaMJ and fixed some time after that in golang.org/x/sys/unix. This explains why syscall and golang.org/x/sys/unix are different. And just fixing one value in syscall might not be enough -- when other ioctl functions are used they could be wrong also. |
|
So it looks like they were always wrong, and it would be good to fix this since so many projects do still syscall. I see that the rec is definitely to move (https://groups.google.com/forum/#!topic/golang-dev/cEASnHIXmLI), but at least the container-focused projects haven't yet. @bradfitz, I did a quick substitution of syscall for sys in parts of the runc code, and it looks like there are some things missing from sys like Signal. Actually, it looks like syscall.Signal is used by sys, so, there's no clean break from syscall. Is this something I should take up on the mailing list if I want to get advice on moving over? It seems strange to mix the two. |
|
I can't think of a reason for golang.org/x/sys/unix to use |
|
I've been trying to understand how sys/unix could be fixed, but I can't get the generator scripts to work. Here are the errors I get when I run this: |
|
What do the definitions for those symbols look like in the header files on your system? |
|
CL https://golang.org/cl/39450 mentions this issue. |
|
@ianlancetaylor Sorry somehow I missed your previous question. The errors above occur because these values are defined in terms of the _IOR macro and the set of header files as they are now don't provide the definition of that before it is used AFAICT. What we have discovered is that on some platforms these are defined in asm/termios.h but on others it is in asm/termbits.h. So when files are changed to use the right header files for ppc64x, that results in many other changes in the zppc64.go files as well as other platforms I'm sure. Something like this seems to make it work by changing mkerrors.sh, as well as a change with the same effect in linux_types.go: |
The comments in this package state that users should be migrating code that uses the syscall package to its corresponding package in x/sys. However, the syscall.Signal and syscall.Errno types and the syscall.SysProcAttr struct is not defined in the x/sys package and still need to be referenced from within syscall. This adds a change to the comments to clarify that the migration will need to continue to use some references to syscall for now. Fixes golang#19560 Change-Id: I8abb96b93bea90070ce461da16dc7bcf7b4b29c1 Reviewed-on: https://go-review.googlesource.com/39450 Reviewed-by: Rob Pike <r@golang.org>
|
Change https://golang.org/cl/66510 mentions this issue: |
Correcting values is allowed per the syscall package rules, so update these constants to their correct value on ppc64/ppc64le. The values now match the corresponding constants in x/sys/unix. Update #19560 Fixes #22000 Change-Id: I1d358de345766ec96e15dfcc8911fe2f39fb0ddb Reviewed-on: https://go-review.googlesource.com/66510 Run-TryBot: Ian Lance Taylor <iant@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Lynn Boger <laboger@linux.vnet.ibm.com> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Please answer these questions before submitting your issue. Thanks!
What version of Go are you using (
go version)?go 1.8, master
What operating system and processor architecture are you using (
go env)?Ubuntu and RH ppc64le and ppc64
What did you do?
Tried to use ioctl syscall with TCGETS on ppc64le.
What did you expect to see?
Valid result.
What did you see instead?
Error message: inappropriate ioctl for device
This error occurs when using the syscall package but works when using golang.org/x/sys/unix due to the fact that these two packages define different values for TCGETS and TCSETS:
In golang syscall package, ztypes_linux_ppc64le.go and ztypes_linux_ppc64.go:
In golang.org/sys/unix/zerrors_linux_ppc64le.go and zerrors_linux_ppc64.go
TCGETS = 0x402c7413
TCSETS = 0x802c7414
I see there is a comment in the golang.org documentation about the use of the syscall package being "locked down". My assumption is that the syscall package should be fixed since it is now incorrect.
Testcase:
ioctlbug.go.txt
Failing execution:
Error on get: inappropriate ioctl for device
importing from golang.org/x/sys/unix allows the test to work.
The text was updated successfully, but these errors were encountered: