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

2627 fix "some of" golangci-lint problems #2641

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 67 additions & 44 deletions contrib/cmd/recvtty/recvtty.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,119 +61,142 @@ terminals:
)

func bail(err error) {
fmt.Fprintf(os.Stderr, "[recvtty] fatal error: %v\n", err)
_, _ = fmt.Fprintf(os.Stderr, "[recvtty] fatal error: %v\n", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is not inline with what the commit message says. Please split your commits carefully.

os.Exit(1)
}

func handleSingle(path string, noStdin bool) error {
func handleSingle(path string, noStdin bool) (retErr error) {
// Open a socket.
ln, err := net.Listen("unix", path)
if err != nil {
return err
ln, retErr := net.Listen("unix", path)
if retErr != nil {
return retErr
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You did't have to change any of this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IOW this can stay err -- the only place to use retErr is the defer.

}
defer ln.Close()
defer func() {
_ = ln.Close()
}()

// We only accept a single connection, since we can only really have
// one reader for os.Stdin. Plus this is all a PoC.
conn, err := ln.Accept()
if err != nil {
return err
conn, retErr := ln.Accept()
if retErr != nil {
return
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

}
defer conn.Close()
defer func() {
_ = conn.Close()
}()

// Close ln, to allow for other instances to take over.
ln.Close()
_ = ln.Close()

// Get the fd of the connection.
unixconn, ok := conn.(*net.UnixConn)
if !ok {
return fmt.Errorf("failed to cast to unixconn")
}

socket, err := unixconn.File()
if err != nil {
return err
socket, retErr := unixconn.File()
if retErr != nil {
return
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

}
defer socket.Close()
defer func() {
_ = socket.Close()
}()

// Get the master file descriptor from runC.
master, err := utils.RecvFd(socket)
if err != nil {
return err
master, retErr := utils.RecvFd(socket)
if retErr != nil {
return
}
c, err := console.ConsoleFromFile(master)
if err != nil {
return err
c, retErr := console.ConsoleFromFile(master)
if retErr != nil {
return
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

}
if err := console.ClearONLCR(c.Fd()); err != nil {
return err
}

// Copy from our stdio to the master fd.
quitChan := make(chan struct{})
errChan := make(chan error)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK it seems that in this case a single channel is enough, there is no need to have two.

go func() {
io.Copy(os.Stdout, c)
_, err := io.Copy(os.Stdout, c)
if err != nil {
errChan <- err
}
quitChan <- struct{}{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IOW this could be simple

_, err := io.Copy(os.Stdout, c)
chan <- err

and the code that's receiving this will check for nil

}()
if !noStdin {
go func() {
io.Copy(c, os.Stdin)
_, err := io.Copy(c, os.Stdin)
if err != nil {
errChan <- err
}
quitChan <- struct{}{}
}()
}

// Only close the master fd once we've stopped copying.
<-quitChan
c.Close()
return nil
select {
case retErr = <-errChan:
return retErr
case <-quitChan:
retErr = c.Close()
return retErr
}
}

func handleNull(path string) error {
func handleNull(path string) (retErr error) {
// Open a socket.
ln, err := net.Listen("unix", path)
if err != nil {
return err
ln, retErr := net.Listen("unix", path)
if retErr != nil {
return retErr
}
defer ln.Close()
defer func() {
_ = ln.Close()
}()

// As opposed to handleSingle we accept as many connections as we get, but
// we don't interact with Stdin at all (and we copy stdout to /dev/null).
for {
conn, err := ln.Accept()
if err != nil {
return err
conn, retErr := ln.Accept()
if retErr != nil {
return retErr
}
go func(conn net.Conn) {
// Don't leave references lying around.
defer conn.Close()
defer func() {
_ = conn.Close()
}()

// Get the fd of the connection.
unixconn, ok := conn.(*net.UnixConn)
if !ok {
return
}

socket, err := unixconn.File()
if err != nil {
socket, retErr := unixconn.File()
if retErr != nil {
return
}
defer socket.Close()
defer func() {
_ = socket.Close()
}()

// Get the master file descriptor from runC.
master, err := utils.RecvFd(socket)
if err != nil {
master, retErr := utils.RecvFd(socket)
if retErr != nil {
return
}

// Just do a dumb copy to /dev/null.
devnull, err := os.OpenFile("/dev/null", os.O_RDWR, 0)
if err != nil {
devnull, retErr := os.OpenFile("/dev/null", os.O_RDWR, 0)
if retErr != nil {
// TODO: Handle this nicely.
return
}

io.Copy(devnull, master)
devnull.Close()
_, _ = io.Copy(devnull, master)
_ = devnull.Close()
}(conn)
}
}
Expand Down
2 changes: 2 additions & 0 deletions libcontainer/cgroups/ebpf/ebpf.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,12 @@ func LoadAttachCgroupDeviceFilter(insts asm.Instructions, license string, dirFD
if err != nil {
return nilCloser, err
}
//nolint:staticcheck
if err := prog.Attach(dirFD, ebpf.AttachCGroupDevice, unix.BPF_F_ALLOW_MULTI); err != nil {
return nilCloser, errors.Wrap(err, "failed to call BPF_PROG_ATTACH (BPF_CGROUP_DEVICE, BPF_F_ALLOW_MULTI)")
}
closer := func() error {
//nolint:staticcheck
if err := prog.Detach(dirFD, ebpf.AttachCGroupDevice, unix.BPF_F_ALLOW_MULTI); err != nil {
return errors.Wrap(err, "failed to call BPF_PROG_DETACH (BPF_CGROUP_DEVICE, BPF_F_ALLOW_MULTI)")
}
Expand Down
6 changes: 3 additions & 3 deletions libcontainer/cgroups/fs/name.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,12 @@ func (s *NameGroup) Name() string {
return s.GroupName
}

func (s *NameGroup) Apply(path string, d *cgroupData) error {
func (s *NameGroup) Apply(path string, d *cgroupData) (retErr error) {
if s.Join {
// ignore errors if the named cgroup does not exist
join(path, d.pid)
retErr = join(path, d.pid)
}
return nil
return
}

func (s *NameGroup) Set(path string, cgroup *configs.Cgroup) error {
Expand Down
4 changes: 2 additions & 2 deletions libcontainer/cgroups/systemd/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -333,11 +333,11 @@ func startUnit(dbusConnection *systemdDbus.Conn, unitName string, properties []s
close(statusChan)
// Please refer to https://godoc.org/github.com/coreos/go-systemd/dbus#Conn.StartUnit
if s != "done" {
dbusConnection.ResetFailedUnit(unitName)
_ = dbusConnection.ResetFailedUnit(unitName)
return errors.Errorf("error creating systemd unit `%s`: got `%s`", unitName, s)
}
case <-timeout.C:
dbusConnection.ResetFailedUnit(unitName)
_ = dbusConnection.ResetFailedUnit(unitName)
return errors.New("Timeout waiting for systemd to create " + unitName)
}
} else if !isUnitExists(err) {
Expand Down
2 changes: 1 addition & 1 deletion libcontainer/cgroups/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,7 @@ func RemovePaths(paths map[string]string) (err error) {
}
}
if len(paths) == 0 {
//nolint:ineffassign // done to help garbage collecting: opencontainers/runc#2506
//nolint:staticcheck,ineffassign // done to help garbage collecting: opencontainers/runc#2506
paths = make(map[string]string)
return nil
}
Expand Down
4 changes: 2 additions & 2 deletions libcontainer/configs/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -385,8 +385,8 @@ func (c Command) Run(s *specs.State) error {
case err := <-errC:
return err
case <-timerCh:
cmd.Process.Kill()
cmd.Wait()
_ = cmd.Process.Kill()
_ = cmd.Wait()
return fmt.Errorf("hook ran past specified timeout of %.1fs", c.Timeout.Seconds())
}
}
7 changes: 5 additions & 2 deletions libcontainer/configs/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,10 @@ func TestFuncHookRun(t *testing.T) {
return nil
})

fHook.Run(state)
err := fHook.Run(state)
if err != nil {
t.Errorf("Unexpected error running hook: %+v", err)
}
}

func TestCommandHookRun(t *testing.T) {
Expand Down Expand Up @@ -184,7 +187,7 @@ func TestCommandHookRunTimeout(t *testing.T) {
Pid: 1,
Bundle: "/bundle",
}
timeout := (10 * time.Millisecond)
timeout := 10 * time.Millisecond

cmdHook := configs.NewCommandHook(configs.Command{
Path: os.Args[0],
Expand Down
37 changes: 20 additions & 17 deletions libcontainer/container_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -680,8 +680,7 @@ var criuFeatures *criurpc.CriuFeatures

func (c *linuxContainer) checkCriuFeatures(criuOpts *CriuOpts, rpcOpts *criurpc.CriuOpts, criuFeat *criurpc.CriuFeatures) error {

var t criurpc.CriuReqType
t = criurpc.CriuReqType_FEATURE_CHECK
var t criurpc.CriuReqType = criurpc.CriuReqType_FEATURE_CHECK

// make sure the features we are looking for are really not from
// some previous check
Expand Down Expand Up @@ -764,10 +763,7 @@ func (c *linuxContainer) checkCriuVersion(minVersion int) error {
const descriptorsFilename = "descriptors.json"

func (c *linuxContainer) addCriuDumpMount(req *criurpc.CriuReq, m *configs.Mount) {
mountDest := m.Destination
if strings.HasPrefix(mountDest, c.config.Rootfs) {
mountDest = mountDest[len(c.config.Rootfs):]
}
mountDest := strings.TrimPrefix(m.Destination, c.config.Rootfs)

extMnt := &criurpc.ExtMountMap{
Key: proto.String(mountDest),
Expand Down Expand Up @@ -1108,10 +1104,7 @@ func (c *linuxContainer) Checkpoint(criuOpts *CriuOpts) error {
}

func (c *linuxContainer) addCriuRestoreMount(req *criurpc.CriuReq, m *configs.Mount) {
mountDest := m.Destination
if strings.HasPrefix(mountDest, c.config.Rootfs) {
mountDest = mountDest[len(c.config.Rootfs):]
}
mountDest := strings.TrimPrefix(m.Destination, c.config.Rootfs)

extMnt := &criurpc.ExtMountMap{
Key: proto.String(mountDest),
Expand Down Expand Up @@ -1216,7 +1209,7 @@ func (c *linuxContainer) prepareCriuRestoreMounts(mounts []*configs.Mount) error
return nil
}

func (c *linuxContainer) Restore(process *Process, criuOpts *CriuOpts) error {
func (c *linuxContainer) Restore(process *Process, criuOpts *CriuOpts) (err error) {
c.m.Lock()
defer c.m.Unlock()

Expand All @@ -1243,15 +1236,20 @@ func (c *linuxContainer) Restore(process *Process, criuOpts *CriuOpts) error {
if err != nil {
return err
}
defer workDir.Close()
defer func() {
err = workDir.Close()
}()

if criuOpts.ImagesDirectory == "" {
return errors.New("invalid directory to restore checkpoint")
}
imageDir, err := os.Open(criuOpts.ImagesDirectory)
if err != nil {
return err
}
defer imageDir.Close()
defer func() {
err = imageDir.Close()
}()
// CRIU has a few requirements for a root directory:
// * it must be a mount point
// * its parent must not be overmounted
Expand All @@ -1261,7 +1259,9 @@ func (c *linuxContainer) Restore(process *Process, criuOpts *CriuOpts) error {
if err := os.Mkdir(root, 0755); err != nil {
return err
}
defer os.Remove(root)
defer func() {
err = os.Remove(root)
}()
root, err = filepath.EvalSymlinks(root)
if err != nil {
return err
Expand All @@ -1270,7 +1270,9 @@ func (c *linuxContainer) Restore(process *Process, criuOpts *CriuOpts) error {
if err != nil {
return err
}
defer unix.Unmount(root, unix.MNT_DETACH)
defer func() {
err = unix.Unmount(root, unix.MNT_DETACH)
}()
t := criurpc.CriuReqType_RESTORE
req := &criurpc.CriuReq{
Type: &t,
Expand Down Expand Up @@ -1378,7 +1380,7 @@ func (c *linuxContainer) Restore(process *Process, criuOpts *CriuOpts) error {

// Now that CRIU is done let's close all opened FDs CRIU needed.
for _, fd := range extraFiles {
fd.Close()
_ = fd.Close()
}

return err
Expand Down Expand Up @@ -1592,7 +1594,7 @@ func (c *linuxContainer) criuSwrk(process *Process, req *criurpc.CriuReq, opts *
break
}

criuClientCon.CloseWrite()
_ = criuClientCon.CloseWrite()
// cmd.Wait() waits cmd.goroutines which are used for proxying file descriptors.
// Here we want to wait only the CRIU process.
criuProcessState, err = criuProcess.Wait()
Expand Down Expand Up @@ -1780,6 +1782,7 @@ func (c *linuxContainer) saveState(s *State) (retErr error) {
return os.Rename(tmpFile.Name(), stateFilePath)
}

//nolint:unused
func (c *linuxContainer) deleteState() error {
return os.Remove(filepath.Join(c.root, stateFilename))
}
Expand Down
Loading