From 51fbf3da9ee34a8143df5baeda6032c1747446d2 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Mon, 25 Apr 2022 15:15:52 +0200 Subject: enable gocritic linter The linter ensures a common code style. - use switch/case instead of else if - use if instead of switch/case for single case statement - add space between comment and text - detect the use of defer with os.Exit() - use short form var += "..." instead of var = var + "..." - detect problems with append() ``` newSlice := append(orgSlice, val) ``` This could lead to nasty bugs because the orgSlice will be changed in place if it has enough capacity too hold the new elements. Thus we newSlice might not be a copy. Of course most of the changes are just cosmetic and do not cause any logic errors but I think it is a good idea to enforce a common style. This should help maintainability. Signed-off-by: Paul Holzinger --- libpod/container.go | 2 +- libpod/container_api.go | 3 +-- libpod/container_exec.go | 12 ++++++------ libpod/container_internal_linux.go | 34 +++++++++++++++------------------- libpod/container_stat_linux.go | 7 ++++--- libpod/define/container_inspect.go | 4 ++-- libpod/define/info.go | 2 +- libpod/kube.go | 26 ++++++++++++++++---------- libpod/logs/log.go | 4 ++-- libpod/networking_linux.go | 8 ++++---- libpod/networking_machine.go | 4 ++-- libpod/networking_slirp4netns.go | 4 +++- libpod/oci_conmon_exec_linux.go | 12 +++++------- libpod/oci_conmon_linux.go | 2 +- libpod/options.go | 2 +- libpod/runtime.go | 4 ++++ 16 files changed, 68 insertions(+), 62 deletions(-) (limited to 'libpod') diff --git a/libpod/container.go b/libpod/container.go index 6e2b7f528..3e7ab7b0a 100644 --- a/libpod/container.go +++ b/libpod/container.go @@ -1180,7 +1180,7 @@ func (c *Container) Umask() string { return c.config.Umask } -//Secrets return the secrets in the container +// Secrets return the secrets in the container func (c *Container) Secrets() []*ContainerSecret { return c.config.Secrets } diff --git a/libpod/container_api.go b/libpod/container_api.go index fe41998c0..a6fcf709d 100644 --- a/libpod/container_api.go +++ b/libpod/container_api.go @@ -229,8 +229,7 @@ func (c *Container) Kill(signal uint) error { // This function returns when the attach finishes. It does not hold the lock for // the duration of its runtime, only using it at the beginning to verify state. func (c *Container) Attach(streams *define.AttachStreams, keys string, resize <-chan define.TerminalSize) error { - switch c.LogDriver() { - case define.PassthroughLogging: + if c.LogDriver() == define.PassthroughLogging { return errors.Wrapf(define.ErrNoLogs, "this container is using the 'passthrough' log driver, cannot attach") } if !c.batched { diff --git a/libpod/container_exec.go b/libpod/container_exec.go index d782bebf8..c05e7fd94 100644 --- a/libpod/container_exec.go +++ b/libpod/container_exec.go @@ -817,16 +817,16 @@ func (c *Container) Exec(config *ExecConfig, streams *define.AttachStreams, resi // Please be careful when using this function since it might temporarily unlock // the container when os.RemoveAll($bundlePath) fails with ENOTEMPTY or EBUSY // errors. -func (c *Container) cleanupExecBundle(sessionID string) (Err error) { +func (c *Container) cleanupExecBundle(sessionID string) (err error) { path := c.execBundlePath(sessionID) for attempts := 0; attempts < 50; attempts++ { - Err = os.RemoveAll(path) - if Err == nil || os.IsNotExist(Err) { + err = os.RemoveAll(path) + if err == nil || os.IsNotExist(err) { return nil } - if pathErr, ok := Err.(*os.PathError); ok { - Err = pathErr.Err - if errors.Cause(Err) == unix.ENOTEMPTY || errors.Cause(Err) == unix.EBUSY { + if pathErr, ok := err.(*os.PathError); ok { + err = pathErr.Err + if errors.Cause(err) == unix.ENOTEMPTY || errors.Cause(err) == unix.EBUSY { // give other processes a chance to use the container if !c.batched { if err := c.save(); err != nil { diff --git a/libpod/container_internal_linux.go b/libpod/container_internal_linux.go index 9f8b7c686..31edff762 100644 --- a/libpod/container_internal_linux.go +++ b/libpod/container_internal_linux.go @@ -505,8 +505,7 @@ func (c *Container) generateSpec(ctx context.Context) (*spec.Spec, error) { } for _, o := range namedVol.Options { - switch o { - case "U": + if o == "U" { if err := c.ChangeHostPathOwnership(mountPoint, true, int(hostUID), int(hostGID)); err != nil { return nil, err } @@ -596,8 +595,7 @@ func (c *Container) generateSpec(ctx context.Context) (*spec.Spec, error) { // Check overlay volume options for _, o := range overlayVol.Options { - switch o { - case "U": + if o == "U" { if err := c.ChangeHostPathOwnership(overlayVol.Source, true, int(hostUID), int(hostGID)); err != nil { return nil, err } @@ -2144,11 +2142,9 @@ func (c *Container) makeBindMounts() error { return err } } - } else { - if !c.config.UseImageHosts && c.state.BindMounts["/etc/hosts"] == "" { - if err := c.createHosts(); err != nil { - return errors.Wrapf(err, "error creating hosts file for container %s", c.ID()) - } + } else if !c.config.UseImageHosts && c.state.BindMounts["/etc/hosts"] == "" { + if err := c.createHosts(); err != nil { + return errors.Wrapf(err, "error creating hosts file for container %s", c.ID()) } } @@ -2267,7 +2263,7 @@ rootless=%d base := "/run/secrets" if secret.Target != "" { secretFileName = secret.Target - //If absolute path for target given remove base. + // If absolute path for target given remove base. if filepath.IsAbs(secretFileName) { base = "" } @@ -2351,7 +2347,7 @@ func (c *Container) generateResolvConf() (string, error) { return "", errors.Wrapf(err, "error parsing host resolv.conf") } - dns := make([]net.IP, 0, len(c.runtime.config.Containers.DNSServers)) + dns := make([]net.IP, 0, len(c.runtime.config.Containers.DNSServers)+len(c.config.DNSServer)) for _, i := range c.runtime.config.Containers.DNSServers { result := net.ParseIP(i) if result == nil { @@ -2359,13 +2355,13 @@ func (c *Container) generateResolvConf() (string, error) { } dns = append(dns, result) } - dnsServers := append(dns, c.config.DNSServer...) + dns = append(dns, c.config.DNSServer...) // If the user provided dns, it trumps all; then dns masq; then resolv.conf var search []string switch { - case len(dnsServers) > 0: + case len(dns) > 0: // We store DNS servers as net.IP, so need to convert to string - for _, server := range dnsServers { + for _, server := range dns { nameservers = append(nameservers, server.String()) } default: @@ -2890,11 +2886,11 @@ func (c *Container) generateUserPasswdEntry(addedUID int) (string, error) { func (c *Container) passwdEntry(username string, uid, gid, name, homeDir string) string { s := c.config.PasswdEntry - s = strings.Replace(s, "$USERNAME", username, -1) - s = strings.Replace(s, "$UID", uid, -1) - s = strings.Replace(s, "$GID", gid, -1) - s = strings.Replace(s, "$NAME", name, -1) - s = strings.Replace(s, "$HOME", homeDir, -1) + s = strings.ReplaceAll(s, "$USERNAME", username) + s = strings.ReplaceAll(s, "$UID", uid) + s = strings.ReplaceAll(s, "$GID", gid) + s = strings.ReplaceAll(s, "$NAME", name) + s = strings.ReplaceAll(s, "$HOME", homeDir) return s + "\n" } diff --git a/libpod/container_stat_linux.go b/libpod/container_stat_linux.go index 9e225de2e..bbe3edbb3 100644 --- a/libpod/container_stat_linux.go +++ b/libpod/container_stat_linux.go @@ -94,15 +94,16 @@ func (c *Container) stat(containerMountPoint string, containerPath string) (*def } } - if statInfo.IsSymlink { + switch { + case statInfo.IsSymlink: // Symlinks are already evaluated and always relative to the // container's mount point. absContainerPath = statInfo.ImmediateTarget - } else if strings.HasPrefix(resolvedPath, containerMountPoint) { + case strings.HasPrefix(resolvedPath, containerMountPoint): // If the path is on the container's mount point, strip it off. absContainerPath = strings.TrimPrefix(resolvedPath, containerMountPoint) absContainerPath = filepath.Join("/", absContainerPath) - } else { + default: // No symlink and not on the container's mount point, so let's // move it back to the original input. It must have evaluated // to a volume or bind mount but we cannot return host paths. diff --git a/libpod/define/container_inspect.go b/libpod/define/container_inspect.go index ae2ce9724..6cdffb8b7 100644 --- a/libpod/define/container_inspect.go +++ b/libpod/define/container_inspect.go @@ -100,7 +100,7 @@ type InspectRestartPolicy struct { // InspectLogConfig holds information about a container's configured log driver type InspectLogConfig struct { Type string `json:"Type"` - Config map[string]string `json:"Config"` //idk type, TODO + Config map[string]string `json:"Config"` // Path specifies a path to the log file Path string `json:"Path"` // Tag specifies a custom log tag for the container @@ -680,7 +680,7 @@ type InspectContainerData struct { SizeRootFs int64 `json:"SizeRootFs,omitempty"` Mounts []InspectMount `json:"Mounts"` Dependencies []string `json:"Dependencies"` - NetworkSettings *InspectNetworkSettings `json:"NetworkSettings"` //TODO + NetworkSettings *InspectNetworkSettings `json:"NetworkSettings"` Namespace string `json:"Namespace"` IsInfra bool `json:"IsInfra"` Config *InspectContainerConfig `json:"Config"` diff --git a/libpod/define/info.go b/libpod/define/info.go index 48ad51c22..713129ada 100644 --- a/libpod/define/info.go +++ b/libpod/define/info.go @@ -12,7 +12,7 @@ type Info struct { Version Version `json:"version"` } -//HostInfo describes the libpod host +// HostInfo describes the libpod host type SecurityInfo struct { AppArmorEnabled bool `json:"apparmorEnabled"` DefaultCapabilities string `json:"capabilities"` diff --git a/libpod/kube.go b/libpod/kube.go index eb62643fe..cbb1a8799 100644 --- a/libpod/kube.go +++ b/libpod/kube.go @@ -220,7 +220,7 @@ func ConvertV1PodToYAMLPod(pod *v1.Pod) *YAMLPod { cs = append(cs, &YAMLContainer{Container: cc, Resources: res}) } mpo := &YAMLPod{Pod: *pod} - mpo.Spec = &YAMLPodSpec{PodSpec: (*pod).Spec, Containers: cs} + mpo.Spec = &YAMLPodSpec{PodSpec: pod.Spec, Containers: cs} for _, ctr := range pod.Spec.Containers { if ctr.SecurityContext == nil || ctr.SecurityContext.SELinuxOptions == nil { continue @@ -527,7 +527,7 @@ func simplePodWithV1Containers(ctx context.Context, ctrs []*Container) (*v1.Pod, // Check if the pod name and container name will end up conflicting // Append -pod if so if util.StringInSlice(podName, ctrNames) { - podName = podName + "-pod" + podName += "-pod" } return newPodObject( @@ -633,7 +633,7 @@ func containerToV1Container(ctx context.Context, c *Container) (v1.Container, [] kubeContainer.Ports = ports // This should not be applicable - //container.EnvFromSource = + // container.EnvFromSource = kubeContainer.SecurityContext = kubeSec kubeContainer.StdinOnce = false kubeContainer.TTY = c.config.Spec.Process.Terminal @@ -885,7 +885,7 @@ func convertVolumePathToName(hostSourcePath string) (string, error) { } // First, trim trailing slashes, then replace slashes with dashes. // Thus, /mnt/data/ will become mnt-data - return strings.Replace(strings.Trim(hostSourcePath, "/"), "/", "-", -1), nil + return strings.ReplaceAll(strings.Trim(hostSourcePath, "/"), "/", "-"), nil } func determineCapAddDropFromCapabilities(defaultCaps, containerCaps []string) *v1.Capabilities { @@ -927,14 +927,20 @@ func capAddDrop(caps *specs.LinuxCapabilities) (*v1.Capabilities, error) { if err != nil { return nil, err } + + defCaps := g.Config.Process.Capabilities // Combine all the default capabilities into a slice - defaultCaps := append(g.Config.Process.Capabilities.Ambient, g.Config.Process.Capabilities.Bounding...) - defaultCaps = append(defaultCaps, g.Config.Process.Capabilities.Effective...) - defaultCaps = append(defaultCaps, g.Config.Process.Capabilities.Inheritable...) - defaultCaps = append(defaultCaps, g.Config.Process.Capabilities.Permitted...) + defaultCaps := make([]string, 0, len(defCaps.Ambient)+len(defCaps.Bounding)+len(defCaps.Effective)+len(defCaps.Inheritable)+len(defCaps.Permitted)) + defaultCaps = append(defaultCaps, defCaps.Ambient...) + defaultCaps = append(defaultCaps, defCaps.Bounding...) + defaultCaps = append(defaultCaps, defCaps.Effective...) + defaultCaps = append(defaultCaps, defCaps.Inheritable...) + defaultCaps = append(defaultCaps, defCaps.Permitted...) // Combine all the container's capabilities into a slice - containerCaps := append(caps.Ambient, caps.Bounding...) + containerCaps := make([]string, 0, len(caps.Ambient)+len(caps.Bounding)+len(caps.Effective)+len(caps.Inheritable)+len(caps.Permitted)) + containerCaps = append(containerCaps, caps.Ambient...) + containerCaps = append(containerCaps, caps.Bounding...) containerCaps = append(containerCaps, caps.Effective...) containerCaps = append(containerCaps, caps.Inheritable...) containerCaps = append(containerCaps, caps.Permitted...) @@ -1042,7 +1048,7 @@ func generateKubeVolumeDeviceFromLinuxDevice(devices []specs.LinuxDevice) []v1.V } func removeUnderscores(s string) string { - return strings.Replace(s, "_", "", -1) + return strings.ReplaceAll(s, "_", "") } // getAutoUpdateAnnotations searches for auto-update container labels diff --git a/libpod/logs/log.go b/libpod/logs/log.go index 0eb3bb922..4d7d5ac58 100644 --- a/libpod/logs/log.go +++ b/libpod/logs/log.go @@ -28,7 +28,7 @@ const ( // FullLogType signifies a log line is full FullLogType = "F" - //ANSIEscapeResetCode is a code that resets all colors and text effects + // ANSIEscapeResetCode is a code that resets all colors and text effects ANSIEscapeResetCode = "\033[0m" ) @@ -167,7 +167,7 @@ func getTailLog(path string, tail int) ([]*LogLine, error) { return tailLog, nil } -//getColor returns a ANSI escape code for color based on the colorID +// getColor returns a ANSI escape code for color based on the colorID func getColor(colorID int64) string { colors := map[int64]string{ 0: "\033[37m", // Light Gray diff --git a/libpod/networking_linux.go b/libpod/networking_linux.go index a312f5a0c..2770b040e 100644 --- a/libpod/networking_linux.go +++ b/libpod/networking_linux.go @@ -579,7 +579,7 @@ func (r *Runtime) GetRootlessNetNs(new bool) (*RootlessNetNS, error) { // lets add /usr/sbin to $PATH ourselves. path = os.Getenv("PATH") if !strings.Contains(path, "/usr/sbin") { - path = path + ":/usr/sbin" + path += ":/usr/sbin" os.Setenv("PATH", path) } @@ -1148,7 +1148,7 @@ func resultToBasicNetworkConfig(result types.StatusBlock) define.InspectBasicNet for _, netAddress := range netInt.Subnets { size, _ := netAddress.IPNet.Mask.Size() if netAddress.IPNet.IP.To4() != nil { - //ipv4 + // ipv4 if config.IPAddress == "" { config.IPAddress = netAddress.IPNet.IP.String() config.IPPrefixLen = size @@ -1157,7 +1157,7 @@ func resultToBasicNetworkConfig(result types.StatusBlock) define.InspectBasicNet config.SecondaryIPAddresses = append(config.SecondaryIPAddresses, define.Address{Addr: netAddress.IPNet.IP.String(), PrefixLength: size}) } } else { - //ipv6 + // ipv6 if config.GlobalIPv6Address == "" { config.GlobalIPv6Address = netAddress.IPNet.IP.String() config.GlobalIPv6PrefixLen = size @@ -1508,7 +1508,7 @@ func ocicniPortsToNetTypesPorts(ports []types.OCICNIPortMapping) []types.PortMap ports[i].Protocol == currentPort.Protocol && ports[i].HostPort-int32(currentPort.Range) == int32(currentPort.HostPort) && ports[i].ContainerPort-int32(currentPort.Range) == int32(currentPort.ContainerPort) { - currentPort.Range = currentPort.Range + 1 + currentPort.Range++ } else { newPorts = append(newPorts, currentPort) currentPort = types.PortMapping{ diff --git a/libpod/networking_machine.go b/libpod/networking_machine.go index 73089c474..7b8eb94df 100644 --- a/libpod/networking_machine.go +++ b/libpod/networking_machine.go @@ -33,9 +33,9 @@ type machineExpose struct { func requestMachinePorts(expose bool, ports []types.PortMapping) error { url := "http://" + machineGvproxyEndpoint + "/services/forwarder/" if expose { - url = url + "expose" + url += "expose" } else { - url = url + "unexpose" + url += "unexpose" } ctx := context.Background() client := &http.Client{ diff --git a/libpod/networking_slirp4netns.go b/libpod/networking_slirp4netns.go index 4a0ef0b3a..788834435 100644 --- a/libpod/networking_slirp4netns.go +++ b/libpod/networking_slirp4netns.go @@ -82,7 +82,9 @@ func checkSlirpFlags(path string) (*slirpFeatures, error) { } func parseSlirp4netnsNetworkOptions(r *Runtime, extraOptions []string) (*slirp4netnsNetworkOptions, error) { - slirpOptions := append(r.config.Engine.NetworkCmdOptions, extraOptions...) + slirpOptions := make([]string, 0, len(r.config.Engine.NetworkCmdOptions)+len(extraOptions)) + slirpOptions = append(slirpOptions, r.config.Engine.NetworkCmdOptions...) + slirpOptions = append(slirpOptions, extraOptions...) slirp4netnsOpts := &slirp4netnsNetworkOptions{ // overwrite defaults disableHostLoopback: true, diff --git a/libpod/oci_conmon_exec_linux.go b/libpod/oci_conmon_exec_linux.go index b8fd82591..6d2f13525 100644 --- a/libpod/oci_conmon_exec_linux.go +++ b/libpod/oci_conmon_exec_linux.go @@ -766,13 +766,11 @@ func prepareProcessExec(c *Container, options *ExecOptions, env []string, sessio if execUser.Uid == 0 { pspec.Capabilities.Effective = pspec.Capabilities.Bounding pspec.Capabilities.Permitted = pspec.Capabilities.Bounding - } else { - if user == c.config.User { - pspec.Capabilities.Effective = ctrSpec.Process.Capabilities.Effective - pspec.Capabilities.Inheritable = ctrSpec.Process.Capabilities.Effective - pspec.Capabilities.Permitted = ctrSpec.Process.Capabilities.Effective - pspec.Capabilities.Ambient = ctrSpec.Process.Capabilities.Effective - } + } else if user == c.config.User { + pspec.Capabilities.Effective = ctrSpec.Process.Capabilities.Effective + pspec.Capabilities.Inheritable = ctrSpec.Process.Capabilities.Effective + pspec.Capabilities.Permitted = ctrSpec.Process.Capabilities.Effective + pspec.Capabilities.Ambient = ctrSpec.Process.Capabilities.Effective } hasHomeSet := false diff --git a/libpod/oci_conmon_linux.go b/libpod/oci_conmon_linux.go index dc1c75cea..c232702e9 100644 --- a/libpod/oci_conmon_linux.go +++ b/libpod/oci_conmon_linux.go @@ -1371,7 +1371,7 @@ func (r *ConmonOCIRuntime) sharedConmonArgs(ctr *Container, cuuid, bundlePath, p case define.JSONLogging: fallthrough //lint:ignore ST1015 the default case has to be here - default: //nolint:stylecheck + default: //nolint:stylecheck,gocritic // No case here should happen except JSONLogging, but keep this here in case the options are extended logrus.Errorf("%s logging specified but not supported. Choosing k8s-file logging instead", ctr.LogDriver()) fallthrough diff --git a/libpod/options.go b/libpod/options.go index 4e597201a..57e2d7cf6 100644 --- a/libpod/options.go +++ b/libpod/options.go @@ -1662,7 +1662,7 @@ func WithTimezone(path string) CtrCreateOption { if err != nil { return err } - //We don't want to mount a timezone directory + // We don't want to mount a timezone directory if file.IsDir() { return errors.New("Invalid timezone: is a directory") } diff --git a/libpod/runtime.go b/libpod/runtime.go index 877e151a9..6c2323d88 100644 --- a/libpod/runtime.go +++ b/libpod/runtime.go @@ -550,6 +550,10 @@ func makeRuntime(runtime *Runtime) (retErr error) { // Check if the pause process was created. If it was created, then // move it to its own systemd scope. utils.MovePauseProcessToScope(pausePid) + + // gocritic complains because defer is not run on os.Exit() + // However this is fine because the lock is released anyway when the process exits + //nolint:gocritic os.Exit(ret) } } -- cgit v1.2.3-54-g00ecf