doc: Fix recently reported issues #181
Conversation
|
New changes are detected. LGTM label has been removed. |
|
I dropped the |
|
@nirs: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: baude, nirs The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
cfergeau
force-pushed
the
restfuluri
branch
4 times, most recently
from
73c9e13 to
a8420d9
Compare
pkg/vf/virtio.go
Outdated
| @@ -202,7 +202,7 @@ func (dev *VirtioRng) AddToVirtualMachineConfig(vmConfig *VirtualMachineConfigur | |||
| // https://developer.apple.com/documentation/virtualization/running_linux_in_a_virtual_machine?language=objc#:~:text=Configure%20the%20Serial%20Port%20Device%20for%20Standard%20In%20and%20Out | |||
| func setRawMode(f *os.File) error { | |||
| // Get settings for terminal | |||
| attr, _ := unix.IoctlGetTermios(int(f.Fd()), unix.TIOCGETA) | |||
| attr, _ := unix.IoctlGetTermios(int(f.Fd()), unix.TIOCGETA) //#nosec G115 | |||
There was a problem hiding this comment.
Is this related? this is not mentioned in the commit message.
There was a problem hiding this comment.
It is the first case in the commit message: « it assumes casting an uintptr file descriptor to int will always be fine ». f.Fd() is an uintptr file descriptor which we cast to int
There was a problem hiding this comment.
I see, always adding a note to the error number will make this more clear:
//#nosec G115 potential integer overflow
I wonder why Go is using uintptr for Fd() when internally it is using int:
os/file_unix.go:
func (f *File) Fd() uintptr {
...
return uintptr(f.pfd.Sysfd)
}
Maybe to handle this issue consistently and avoid littering the code with #nosec notes, with add a helper function like:
func unixFd(f *os.File) int {
// On unix the underlying fd is int, overflow is not possible.
return int(f.Fd()) //#nosec G115 potential integer overflow
}
Ideally we can prove this in compile time type assertion.
There was a problem hiding this comment.
I wonder why Go is using uintptr for Fd() when internally it is using int:
From the look of it, for Windows:
https://github.com/golang/go/blob/96d8ff00c2d6a88384863a656fb5e53716b614d3/src/os/file_windows.go#L34-L45
https://github.com/golang/go/blob/96d8ff00c2d6a88384863a656fb5e53716b614d3/src/internal/poll/fd_windows.go#L226-L227
where Handle is an uintptr: https://cs.opensource.google/go/x/sys/+/master:windows/syscall_windows.go;l=21
There was a problem hiding this comment.
Good points about adding comments explaining what G115 is and about the helper. It's only func unixFd(fd uintptr) int so that it can be used both in virtio.go and virtionet.go, even if it's trivial, it makes the code easier to read.
pkg/vf/vsock.go
Outdated
| @@ -19,13 +20,17 @@ func ExposeVsock(vm *VirtualMachine, port uint, vsockPath string, listen bool) ( | |||
| } | |||
|
|
|||
| func ConnectVsockSync(vm *VirtualMachine, port uint) (net.Conn, error) { | |||
| if port > math.MaxUint32 { | |||
There was a problem hiding this comment.
If port is actually uint32, why accept uint in this function?
There was a problem hiding this comment.
Changing the function arg to uint32 involves changes in quite a few places, including the public pkg/config API, or it means we need to duplicate this check in several places instead of in a single one.
Since it's quite unlikely one will try to use a huge vsock port, I favoured having the check in a single place, and not changing the rest of the code.
Below is a partial diff of the changes we'd need to make to pass the correct uint32 type over the codebase. I haven't tried to compile it so there are most likely more places which need to change.
diff --git a/pkg/config/config.go b/pkg/config/config.go
index d836d34..648e523 100644
--- a/pkg/config/config.go
+++ b/pkg/config/config.go
@@ -23,7 +23,7 @@ type VirtualMachine struct {
// TimeSync enables synchronization of the host time to the linux guest after the host was suspended.
// This requires qemu-guest-agent to be running in the guest, and to be listening on a vsock socket
type TimeSync struct {
- VsockPort uint `json:"vsockPort"`
+ VsockPort uint32 `json:"vsockPort"`
}
// The VMComponent interface represents a VM element (device, bootloader, ...)
@@ -179,7 +179,7 @@ func (vm *VirtualMachine) TimeSync() *TimeSync {
return vm.Timesync
}
-func TimeSyncNew(vsockPort uint) (VMComponent, error) {
+func TimeSyncNew(vsockPort uint32) (VMComponent, error) {
return &TimeSync{
VsockPort: vsockPort,
}, nil
@@ -201,7 +201,7 @@ func (ts *TimeSync) FromOptions(options []option) error {
if err != nil {
return err
}
- ts.VsockPort = uint(vsockPort)
+ ts.VsockPort = uint32(vsockPort)
default:
return fmt.Errorf("unknown option for timesync parameter: %s", option.key)
}
diff --git a/pkg/config/virtio.go b/pkg/config/virtio.go
index cba539a..c7acfb7 100644
--- a/pkg/config/virtio.go
+++ b/pkg/config/virtio.go
@@ -47,7 +47,7 @@ type VirtioGPU struct {
type VirtioVsock struct {
// Port is the virtio-vsock port used for this device, see `man vsock` for more
// details.
- Port uint `json:"port"`
+ Port uint32 `json:"port"`
// SocketURL is the path to a unix socket on the host to use for the virtio-vsock communication with the guest.
SocketURL string `json:"socketURL"`
// If true, vsock connections will have to be done from guest to host. If false, vsock connections will only be possible
@@ -531,7 +531,7 @@ func (dev *VirtioBlk) ToCmdLine() ([]string, error) {
// vsock port, and on the host it will use the unix socket at socketURL.
// When listen is true, the host will be listening for connections over vsock.
// When listen is false, the guest will be listening for connections over vsock.
-func VirtioVsockNew(port uint, socketURL string, listen bool) (VirtioDevice, error) {
+func VirtioVsockNew(port uint32, socketURL string, listen bool) (VirtioDevice, error) {
return &VirtioVsock{
Port: port,
SocketURL: socketURL,
@@ -560,11 +560,11 @@ func (dev *VirtioVsock) FromOptions(options []option) error {
case "socketURL":
dev.SocketURL = option.value
case "port":
- port, err := strconv.Atoi(option.value)
+ port, err := strconv.ParseUint(option.value, 10, 32)
if err != nil {
return err
}
- dev.Port = uint(port)
+ dev.Port = uint32(port)
case "listen":
dev.Listen = true
case "connect":
There was a problem hiding this comment.
Changing the function arg to
uint32involves changes in quite a few places, including the publicpkg/configAPI,
This sounds like the right way to me. We should use the type system to make errors in runtime impossible if we can.
The example diff needed to change the type looks good.
There was a problem hiding this comment.
This sounds like the right way to me. We should use the type system to make errors in runtime impossible if we can.
I'll look into this,but I need to double check this does not break compilation for external users of pkg/config though.
cfergeau
force-pushed
the
restfuluri
branch
2 times, most recently
from
be4f255 to
fe0d39d
Compare
|
I think it will be better to split the commit handling possible integer overflow to another PR, and merge the easy changes now. |
Yep, I was also getting to that conclusion, I initially thought the overflow changes would be a quick fix ;) |
This is #185 |
This fixes crc-org#173 Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
The doc is using 'mibibytes', and 1024*1024*1024, this commit fixes both issues, and adds a link to the wikipedia article. This fixes crc-org#176 Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
cfergeau
changed the title
This fixes minor (confusing) errors in the documentation and some lint errors.