Skip to content
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

x/sys/unix: Reboot takes int but LINUX_REBOOT_CMD_HALT overflows int #9584

Closed
tv42 opened this issue Jan 13, 2015 · 7 comments
Closed

x/sys/unix: Reboot takes int but LINUX_REBOOT_CMD_HALT overflows int #9584

tv42 opened this issue Jan 13, 2015 · 7 comments

Comments

@tv42
Copy link

tv42 commented Jan 13, 2015

package main

import "golang.org/x/sys/unix"

func main() {
    unix.Reboot(unix.LINUX_REBOOT_CMD_HALT)
}
$ GOARCH=amd64 go build halt.go 
$ GOARCH=386 go build halt.go 
# command-line-arguments
./halt.go:6: constant 3454992675 overflows int
$ 
@ianlancetaylor ianlancetaylor changed the title unix: Reboot takes int but LINUX_REBOOT_CMD_HALT overflows int x/sys/unix: Reboot takes int but LINUX_REBOOT_CMD_HALT overflows int Jan 13, 2015
@minux
Copy link
Member

minux commented Jan 14, 2015

you need to cast it to an int.

When generating the constants, there is no way to know whether it's
intended as unsigned constant or signed, esp. those magic numbers.

You might argue that we can change the constants to int, but please
note that you can also use it in this way:
unix.Syscall(unix.SYS_REBOOT, unix.LINUX_REBOOT_CMD_HALT, 0, 0)

And in this case, if the constant were signed, compiler would warn about
the negative constant overflows uintptr.

A more realistic change would be to change unix.Reboot to take a
uintptr. But Reboot is such a uncommon API, I don't think it's worth a
API change just to remove a cast.

@tv42
Copy link
Author

tv42 commented Jan 14, 2015

Actually, that can't even be converted to int conveniently; the compiler is too smart, and sees the large constant. Jumping through hoops is required:

    h := uint(unix.LINUX_REBOOT_CMD_HALT)
    unix.Reboot(int(h))

@minux
Copy link
Member

minux commented Jan 14, 2015

If we make the const signed, then using Syscall will require the same
conversions. And as I've said, when generating the constants, we don't
know they are signed or unsigned, and I don't want to add manual rules
to the process.

@griesemer
Copy link
Contributor

const c = unix.LINUX_REBOOT_CMD_HALT
unix.Reboot(-(c>>31)<<31 | c&(1<<31-1))

:-)

@minux
Copy link
Member

minux commented Jan 14, 2015

Cool!
Robert, will you write a "fun with Go constants" article/blog some day?

@tv42
Copy link
Author

tv42 commented Jan 15, 2015

Why does Reboot have to take an int? I understand it's currently just machine generated, but sys/unix has human-written code already. And if you're saying this kind of Go-friendliness doesn't belong in sys/unix that's just really raw syscalls, well where does it belong?

@minux
Copy link
Member

minux commented Jan 16, 2015

The consts are machine generated, but Reboot function is manually defined
in https://golang.org/cl/4256060.

The question is really whether we should make a breaking change for
convenience
reasons. (In retrospect, we probably don't need to add that CL, as that
doesn't
expose all functionality of the underlying reboot syscall, and it's just as
easy
to use Syscall6 directly instead for such a rarely used syscall.)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants