libcontainer/intelrdt: modify the incorrect file mode by KentaTada · Pull Request #2625 · opencontainers/runc

KentaTada · 2020-10-01T22:45:30Z

This file mode is unintentional apparently.

Signed-off-by: Kenta Tada [email protected]

thaJeztah

LGTM

did the linter catch this? (if not, we may have to update the configuration, as I think it should be able to catch this)

kolyshkin · 2020-10-02T01:52:33Z

Yes, it's a linter error, I've been meaning to fix this but didn't have time yet

kolyshkin · 2020-10-02T01:53:35Z


 		for fileName, value := range mocks {
-			err := ioutil.WriteFile(filepath.Join(numaPath, fileName), []byte(strconv.FormatUint(value, 10)), 777)
+			err := ioutil.WriteFile(filepath.Join(numaPath, fileName), []byte(strconv.FormatUint(value, 10)), 0777)


Just for the sake of sanity, I'd prefer 0644 here. Or, using the latest and greatest golang, 0o644.

I wanted to respect the original commit in this pull request.
But if maintainers don't mind it, I'll change from 0777 to 0o644 in this pull request.

@kolyshkin I changed the file mode. Thanks!

kolyshkin

a nit

KentaTada · 2020-10-02T01:57:09Z

did the linter catch this? (if not, we may have to update the configuration, as I think it should be able to catch this)

Yes.
I also tested runc using linter and I found it.

Signed-off-by: Kenta Tada <[email protected]>

thaJeztah · 2020-10-02T07:43:34Z

Looks like the linter on PR's is perhaps only checking the changed lines; I saw Akihiro's comment on #2618 (comment), and the linter picked up the problem in https://github.com/opencontainers/runc/runs/1190924966

Looks like that output is a good list of things to work on;

Details

Error: `userFromOS` is unused (deadcode)
Error: `groupFromOS` is unused (deadcode)
Error: `loadConfig` is unused (deadcode)
Error: `removePath` is unused (deadcode)
Error: `wildcard` is unused (deadcode)
Error: Error return value of `os.Mkdir` is not checked (errcheck)
Error: Error return value of `os.Symlink` is not checked (errcheck)
Error: Error return value of `console.ClearONLCR` is not checked (errcheck)
Error: Error return value of `io.Copy` is not checked (errcheck)
Error: Error return value of `io.Copy` is not checked (errcheck)
Error: Error return value of `io.Copy` is not checked (errcheck)
Error: Error return value of `cmd.Process.Kill` is not checked (errcheck)
Error: Error return value of `cmd.Wait` is not checked (errcheck)
Error: Error return value of `fHook.Run` is not checked (errcheck)
Error: Error return value of `join` is not checked (errcheck)
Error: Error return value of `os.Symlink` is not checked (errcheck)
Error: Error return value of `dbusConnection.ResetFailedUnit` is not checked (errcheck)
Error: Error return value of `dbusConnection.ResetFailedUnit` is not checked (errcheck)
Error: Error return value of `unix.Unmount` is not checked (errcheck)
Error: Error return value of `criuClientCon.CloseWrite` is not checked (errcheck)
Error: Error return value of `Cgroupfs` is not checked (errcheck)
Error: Error return value of `unix.Unmount` is not checked (errcheck)
Error: Error return value of `m.Freeze` is not checked (errcheck)
Error: Error return value of `p.wait` is not checked (errcheck)
Error: Error return value of `p.cmd.Wait` is not checked (errcheck)
Error: Error return value of `p.cmd.Wait` is not checked (errcheck)
Error: Error return value of `p.cmd.Wait` is not checked (errcheck)
Error: Error return value of `p.manager.Destroy` is not checked (errcheck)
Error: Error return value of `p.intelRdtManager.Destroy` is not checked (errcheck)
Error: Error return value of `p.wait` is not checked (errcheck)
Error: Error return value of `signalAllProcesses` is not checked (errcheck)
Error: Error return value of `unix.Unmount` is not checked (errcheck)
Error: Error return value of `cleanupTmp` is not checked (errcheck)
Error: Error return value of `selinux.SetKeyLabel` is not checked (errcheck)
Error: Error return value of `selinux.SetExecLabel` is not checked (errcheck)
Error: Error return value of `selinux.SetKeyLabel` is not checked (errcheck)
Error: Error return value of `selinux.SetExecLabel` is not checked (errcheck)
Error: Error return value of `i.c.initProcess.signal` is not checked (errcheck)
Error: Error return value of `p.Wait` is not checked (errcheck)
Error: Error return value of `container.Destroy` is not checked (errcheck)
Error: Error return value of `showFile` is not checked (errcheck)
Error: Error return value of `showFile` is not checked (errcheck)
Error: Error return value of `showFile` is not checked (errcheck)
Error: Error return value of `container.Destroy` is not checked (errcheck)
Error: Error return value of `container.Destroy` is not checked (errcheck)
Error: Error return value of `unmountOp` is not checked (errcheck)
Error: Error return value of `unmountOp` is not checked (errcheck)
Error: Error return value of `unmountOp` is not checked (errcheck)
Error: Error return value of `container1.Destroy` is not checked (errcheck)
Error: Error return value of `container2.Destroy` is not checked (errcheck)
Error: Error return value of `container1.Destroy` is not checked (errcheck)
Error: Error return value of `container2.Destroy` is not checked (errcheck)
Error: Error return value of `console.ClearONLCR` is not checked (errcheck)
Error: Error return value of `h.Write` is not checked (errcheck)
Error: Error return value of `client.Write` is not checked (errcheck)
Error: ineffectual assignment to `paths` (ineffassign)
Error: `si_code` is unused (structcheck)
Error: `pad` is unused (structcheck)
Error: `si_signo` is unused (structcheck)
Error: `si_errno` is unused (structcheck)
Error: S1002: should omit comparison to bool constant, can be simplified to `ok` (gosimple)
Error: S1002: should omit comparison to bool constant, can be simplified to `ok` (gosimple)
Error: S1002: should omit comparison to bool constant, can be simplified to `ok` (gosimple)
Error: S1002: should omit comparison to bool constant, can be simplified to `!symLink` (gosimple)
Error: S1032: should use sort.Ints(...) instead of sort.Sort(sort.IntSlice(...)) (gosimple)
Error: S1032: should use sort.Ints(...) instead of sort.Sort(sort.IntSlice(...)) (gosimple)
Error: S1023: redundant `return` statement (gosimple)
Error: S1023: redundant `return` statement (gosimple)
Error: S1017: should replace this `if` statement with an unconditional `strings.TrimPrefix` (gosimple)
Error: S1017: should replace this `if` statement with an unconditional `strings.TrimPrefix` (gosimple)
Error: S1006: should use for {} instead of for true {} (gosimple)
Error: S1008: should use 'return err == nil' instead of 'if err != nil { return false }; return true' (gosimple)
Error: S1021: should merge variable declaration with assignment on next line (gosimple)
Error: S1021: should merge variable declaration with assignment on next line (gosimple)
Error: S1030: should use stdout.String() instead of string(stdout.Bytes()) (gosimple)
Error: S1030: should use stdout.String() instead of string(stdout.Bytes()) (gosimple)
Error: S1030: should use stdout2.String() instead of string(stdout2.Bytes()) (gosimple)
Error: S1030: should use stdout.String() instead of string(stdout.Bytes()) (gosimple)
Error: S1030: should use stdout2.String() instead of string(stdout2.Bytes()) (gosimple)
Error: SA4006: this value of `process` is never used (staticcheck)
Error: SA4000: identical expressions on the left and right side of the '-' operator (staticcheck)
Error: SA5001: should check returned error before deferring file.Close() (staticcheck)
Error: SA9002: file mode '777' evaluates to 01411; did you mean '0777'? (staticcheck)
Error: SA1019: prog.Attach is deprecated: use link.RawAttachProgram instead.  (staticcheck)
Error: SA1019: prog.Detach is deprecated: use link.RawDetachProgram instead.  (staticcheck)
Error: SA1019: package github.com/golang/protobuf/proto is deprecated: Use the "google.golang.org/protobuf/proto" package instead.  (staticcheck)
Error: func `(*linuxContainer).deleteState` is unused (unused)

Error: issues found

Some notes there;

I'm not sure it the logs output all errors, or a limited list; usually, golang-ci-lint is configured to limit some issues if they occur too many times
Depending on the above, may consider disabling errcheck, or configure a rule/regex for error-handling we think is acceptable to ignore

KentaTada · 2020-10-02T07:53:13Z

I'm not sure it the logs output all errors, or a limited list; usually, golang-ci-lint is configured to limit some issues if they occur too many times

Yes.

For example,

Error: userFromOS is unused (deadcode)
Error: groupFromOS is unused (deadcode)

I think those functions are actually used for windows environment. Doesn't the current CI script recognize windows environment?

thaJeztah · 2020-10-02T07:58:54Z

I think those functions are actually used for windows environment. Doesn't the current CI script recognize windows environment?

Not sure, it may be only checking for the platform it runs on. IIRC, there's also an option to specify which build tags to use when linting. (so multiple combinations of build-tags could be needed to lint everything).

These should also be ignored likely (but could potentially be candidates for inclusion in golang.org/x/sys);

Error: `si_code` is unused (structcheck)
Error: `pad` is unused (structcheck)
Error: `si_signo` is unused (structcheck)
Error: `si_errno` is unused (structcheck)

thaJeztah · 2020-10-02T08:02:49Z

Let me open a new ticket for tracking linting issues

KentaTada · 2020-10-02T08:05:41Z

Let me open a new ticket for tracking linting issues

Thanks!

thaJeztah · 2020-10-02T08:08:13Z

Opened #2627 as a quick first draft 👍

thaJeztah · 2020-10-02T09:34:52Z

For consistency, would be good if we did a look for all 0755, 0644, 0777 and change to the new 0o... format (possibly look if perms are correct as well, e.g. in some cases 0700 or 0600 could be correct if we expect files to be exclusively used by runc?)

This commit adjusts the file mode to use the latest golang style Related to opencontainers#2625 Signed-off-by: Kenta Tada <[email protected]>

This commit adjusts the file mode to use the latest golang style In addition to that, I changed those modes from 0700 to 0600 as same as opencontainers#2636 Related to opencontainers#2625 Signed-off-by: Kenta Tada <[email protected]>

This commit adjusts the file mode to use the latest golang style Related to opencontainers#2625 Signed-off-by: Kenta Tada <[email protected]>

This commit adjusts the file mode to use the latest golang style In addition to that, I changed those modes from 0700 to 0600 as same as opencontainers#2636 Related to opencontainers#2625 Signed-off-by: Kenta Tada <[email protected]>

thaJeztah previously approved these changes Oct 1, 2020

View reviewed changes

kolyshkin reviewed Oct 2, 2020

View reviewed changes

kolyshkin requested changes Oct 2, 2020

View reviewed changes

libcontainer/intelrdt: modify the incorrect file mode

644c107

Signed-off-by: Kenta Tada <[email protected]>

KentaTada dismissed thaJeztah’s stale review via 644c107 October 2, 2020 02:08

KentaTada force-pushed the modify-rdt-test-perm branch from 1f5089f to 644c107 Compare October 2, 2020 02:08

AkihiroSuda approved these changes Oct 2, 2020

View reviewed changes

thaJeztah mentioned this pull request Oct 2, 2020

Review and address golang-ci linting errors #2627

Closed

kolyshkin approved these changes Oct 3, 2020

View reviewed changes

kolyshkin merged commit dc775c1 into opencontainers:master Oct 3, 2020

KentaTada mentioned this pull request Oct 6, 2020

libcontainer/intelrdt: adjust the file mode #2636

Merged

KentaTada pushed a commit to KentaTada/runc that referenced this pull request Mar 2, 2021

libcontainer/factory*: adjust the file mode

8a3484b

This commit adjusts the file mode to use the latest golang style Related to opencontainers#2625 Signed-off-by: Kenta Tada <[email protected]>

KentaTada mentioned this pull request Mar 2, 2021

libcontainer/factory*: adjust the file mode #2829

Merged

KentaTada mentioned this pull request Mar 8, 2021

libcontainer/notify_linux*: adjust the file mode #2837

Closed

KentaTada pushed a commit to KentaTada/runc that referenced this pull request Mar 8, 2021

libcontainer/configs: adjust the file mode

e56a658

This commit adjusts the file mode to use the latest golang style Related to opencontainers#2625 Signed-off-by: Kenta Tada <[email protected]>

KentaTada pushed a commit to KentaTada/runc that referenced this pull request Mar 8, 2021

libcontainer/integration: adjust the file mode

f67e630

This commit adjusts the file mode to use the latest golang style Related to opencontainers#2625 Signed-off-by: Kenta Tada <[email protected]>

This was referenced Mar 8, 2021

libcontainer/configs: adjust the file mode #2838

Closed

libcontainer/integration: adjust the file mode #2839

Closed

Conversation

KentaTada commented Oct 1, 2020

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented Oct 2, 2020

Uh oh!

kolyshkin Oct 2, 2020

Choose a reason for hiding this comment

Uh oh!

KentaTada Oct 2, 2020

Choose a reason for hiding this comment

Uh oh!

KentaTada Oct 2, 2020

Choose a reason for hiding this comment

Uh oh!

kolyshkin left a comment

Choose a reason for hiding this comment

Uh oh!

KentaTada commented Oct 2, 2020

Uh oh!

thaJeztah commented Oct 2, 2020

Uh oh!

KentaTada commented Oct 2, 2020

Uh oh!

thaJeztah commented Oct 2, 2020

Uh oh!

thaJeztah commented Oct 2, 2020

Uh oh!

KentaTada commented Oct 2, 2020

Uh oh!

thaJeztah commented Oct 2, 2020

Uh oh!

thaJeztah commented Oct 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants