Add more go vet checks

.golangci.yml

@@ -21,17 +21,31 @@ linters:
  # - unused
 
  - gofmt # whether code was gofmt-ed
+ - govet # enabled by default, but just to be sure
  - nolintlint # ill-formed or insufficient nolint directives
  - stylecheck # golint replacement
  - thelper # test helpers without t.Helper()
 
 linters-settings:
+ govet:
+ enable-all: true
+ disable:
+ # struct order is often for Win32 compat
+ # also, ignore pointer bytes/GC issues for now until performance becomes an issue
+ - fieldalignment
+ check-shadowing: true
+
  stylecheck:
  # https://staticcheck.io/docs/checks
  checks: ["all"]
 
 issues:
  exclude-rules:
+ # err is very often shadowed in nested scopes
+ - linters:
+ - govet
+ text: '^shadow: declaration of "err" shadows declaration'
+
  # path is relative to module root, which is ./test/
  - path: cri-containerd
  linters:

cmd/containerd-shim-runhcs-v1/service.go

@@ -511,7 +511,7 @@ func (s *service) DiagStacks(ctx context.Context, req *shimdiag.StacksRequest) (
 
  t, _ := s.getTask(s.tid)
  if t != nil {
- ctx, cancel := context.WithTimeout(ctx, 5*time.Second)
+ ctx, cancel := context.WithTimeout(ctx, 5*time.Second) //nolint:govet // shadow
  defer cancel()
  resp.GuestStacks = t.DumpGuestStacks(ctx)
  }

cmd/ncproxy/service.go

@@ -47,8 +47,8 @@ const (
  serviceConfigFailureActions = 2
 )
 
-func initPanicFile(path string) error {
- panicFile, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0644)
+func initPanicFile(path string) (err error) {
+ panicFile, err = os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0644)
  if err != nil {
  return err
  }
@@ -92,6 +92,10 @@ func initPanicFile(path string) error {
 }
 
 func removePanicFile() {
+ if panicFile == nil {
+ // shouldn't get here, but somehow launchService was called before initPanicFile
+ return
+ }
  if st, err := panicFile.Stat(); err == nil {
  // If there's anything in the file we wrote (e.g. panic logs), don't delete it.
  if st.Size() == 0 {

internal/exec/exec.go

@@ -176,7 +176,7 @@ func (e *Exec) Start() error {
  pSec := &windows.SecurityAttributes{Length: uint32(unsafe.Sizeof(zeroSec)), InheritHandle: 1}
  tSec := &windows.SecurityAttributes{Length: uint32(unsafe.Sizeof(zeroSec)), InheritHandle: 1}
 
- siEx.ProcThreadAttributeList = e.attrList.List()
+ siEx.ProcThreadAttributeList = e.attrList.List() //nolint:govet // unusedwrite: ProcThreadAttributeList will be read in syscall
  siEx.Cb = uint32(unsafe.Sizeof(*siEx))
  if e.execConfig.token != 0 {
  err = windows.CreateProcessAsUser(

internal/jobobject/jobobject.go

@@ -167,7 +167,7 @@ func Create(ctx context.Context, options *Options) (_ *JobObject, err error) {
 //
 // Returns a JobObject structure and an error if there is one.
 func Open(ctx context.Context, options *Options) (_ *JobObject, err error) {
- if options == nil || (options != nil && options.Name == "") {
+ if options == nil || options.Name == "" {
  return nil, errors.New("no job object name specified to open")
  }
 

internal/layers/layers.go

@@ -52,7 +52,7 @@ type lcowLayersCloser struct {
 func (lc *lcowLayersCloser) Release(ctx context.Context) (retErr error) {
  if err := lc.uvm.RemoveCombinedLayersLCOW(ctx, lc.guestCombinedLayersPath); err != nil {
  log.G(ctx).WithError(err).Error("failed RemoveCombinedLayersLCOW")
- if retErr == nil {
+ if retErr == nil { //nolint:govet // nilness: consistency with below
  retErr = fmt.Errorf("first error: %w", err)
  }
  }
@@ -307,7 +307,7 @@ type wcowIsolatedLayersCloser struct {
 func (lc *wcowIsolatedLayersCloser) Release(ctx context.Context) (retErr error) {
  if err := lc.uvm.RemoveCombinedLayersWCOW(ctx, lc.guestCombinedLayersPath); err != nil {
  log.G(ctx).WithError(err).Error("failed RemoveCombinedLayersWCOW")
- if retErr == nil {
+ if retErr == nil { //nolint:govet // nilness: consistency with below
  retErr = fmt.Errorf("first error: %w", err)
  }
  }

internal/log/scrub.go

@@ -89,11 +89,11 @@ func scrubBridgeCreate(m genMap) error {
 }
 
 func scrubLinuxHostedSystem(m genMap) error {
- if m, ok := index(m, "OciSpecification"); ok {
+ if m, ok := index(m, "OciSpecification"); ok { //nolint:govet // shadow
  if _, ok := m["annotations"]; ok {
  m["annotations"] = map[string]string{_scrubbedReplacement: _scrubbedReplacement}
  }
- if m, ok := index(m, "process"); ok {
+ if m, ok := index(m, "process"); ok { //nolint:govet // shadow
  if _, ok := m["env"]; ok {
  m["env"] = []string{_scrubbedReplacement}
  return nil
@@ -113,7 +113,7 @@ func scrubExecuteProcess(m genMap) error {
  if !isRequestBase(m) {
  return ErrUnknownType
  }
- if m, ok := index(m, "Settings"); ok {
+ if m, ok := index(m, "Settings"); ok { //nolint:govet // shadow
  if ss, ok := m["ProcessParameters"]; ok {
  // ProcessParameters is a json encoded struct passed as a regular sting field
  s, ok := ss.(string)

internal/regopolicyinterpreter/regopolicyinterpreter.go

@@ -214,7 +214,7 @@ func (r *RegoPolicyInterpreter) GetMetadata(name string, key string) (interface{
 
  if metadata, ok := metadataRoot[name]; ok {
  if value, ok := metadata[key]; ok {
- value, err := copyValue(value)
+ value, err := copyValue(value) //nolint:govet // shadow
  if err != nil {
  return nil, fmt.Errorf("unable to copy value: %w", err)
  }

internal/safefile/safeopen.go

@@ -276,7 +276,7 @@ func RemoveAllRelative(path string, root *os.File) error {
  }
 
  // It is necessary to use os.Open as Readdirnames does not work with
- // OpenRelative. This is safe because the above lstatrelative fails
+ // OpenRelative. This is safe because the above LstatRelative fails
  // if the target is outside the root, and we know this is not a
  // symlink from the above FILE_ATTRIBUTE_REPARSE_POINT check.
  fd, err := os.Open(filepath.Join(root.Name(), path))
@@ -293,12 +293,12 @@ func RemoveAllRelative(path string, root *os.File) error {
  for {
  names, err1 := fd.Readdirnames(100)
  for _, name := range names {
- err1 := RemoveAllRelative(path+string(os.PathSeparator)+name, root)
- if err == nil {
- err = err1
+ if err2 := RemoveAllRelative(path+string(os.PathSeparator)+name, root); err == nil {
+ err = err2
  }
  }
  if err1 == io.EOF {
+ // Readdirnames has no more files to return
  break
  }
  // If Readdirnames returned an error, use it.

internal/tools/networkagent/main.go

@@ -259,7 +259,7 @@ func (s *service) teardownConfigureContainerNetworking(ctx context.Context, req
 
  for _, endpoint := range resp.Endpoints {
  if endpoint == nil {
- log.G(ctx).WithField("name", endpoint.ID).Warn("failed to find endpoint to delete")
+ log.G(ctx).Warn("failed to find endpoint to delete")
  continue
  }
  if endpoint.Endpoint == nil || endpoint.Endpoint.Settings == nil {
@@ -340,7 +340,7 @@ func (s *service) addHelper(ctx context.Context, req *nodenetsvc.ConfigureNetwor
 
  for _, endpoint := range resp.Endpoints {
  if endpoint == nil {
- log.G(ctx).WithField("name", endpoint.ID).Warn("failed to find endpoint")
+ log.G(ctx).Warn("failed to find endpoint")
  continue
  }
  if endpoint.Endpoint == nil || endpoint.Endpoint.Settings == nil {
@@ -393,7 +393,7 @@ func (s *service) teardownHelper(ctx context.Context, req *nodenetsvc.ConfigureN
 
  for _, endpoint := range resp.Endpoints {
  if endpoint == nil {
- log.G(ctx).WithField("name", endpoint.ID).Warn("failed to find endpoint to delete")
+ log.G(ctx).Warn("failed to find endpoint to delete")
  continue
  }
  if endpoint.Endpoint == nil || endpoint.Endpoint.Settings == nil {

internal/uvm/create_test.go

@@ -24,7 +24,7 @@ func TestCreateWCOWBadLayerFolders(t *testing.T) {
  opts := NewDefaultOptionsWCOW(t.Name(), "")
  _, err := CreateWCOW(context.Background(), opts)
  errMsg := fmt.Sprintf("%s: %s", errBadUVMOpts, "at least 2 LayerFolders must be supplied")
- if err == nil || (err != nil && err.Error() != errMsg) {
+ if err == nil || err.Error() != errMsg {
  t.Fatal(err)
  }
 }

internal/uvm/update_uvm.go

@@ -22,7 +22,7 @@ func (uvm *UtilityVM) Update(ctx context.Context, data interface{}, annots map[s
  memoryLimitInBytes = resources.Memory.Limit
  }
  if resources.CPU != nil {
- processorLimits := &hcsschema.ProcessorLimits{}
+ processorLimits = &hcsschema.ProcessorLimits{}
  if resources.CPU.Maximum != nil {
  processorLimits.Limit = uint64(*resources.CPU.Maximum)
  }
@@ -36,7 +36,7 @@ func (uvm *UtilityVM) Update(ctx context.Context, data interface{}, annots map[s
  memoryLimitInBytes = &mem
  }
  if resources.CPU != nil {
- processorLimits := &hcsschema.ProcessorLimits{}
+ processorLimits = &hcsschema.ProcessorLimits{}
  if resources.CPU.Quota != nil {
  processorLimits.Limit = uint64(*resources.CPU.Quota)
  }

internal/vm/remotevm/builder.go

@@ -29,7 +29,7 @@ type utilityVMBuilder struct {
  client vmservice.VMService
 }
 
-func NewUVMBuilder(ctx context.Context, id, owner, binPath, addr string, guestOS vm.GuestOS) (vm.UVMBuilder, error) {
+func NewUVMBuilder(ctx context.Context, id, owner, binPath, addr string, guestOS vm.GuestOS) (_ vm.UVMBuilder, err error) {
  var job *jobobject.JobObject
  if binPath != "" {
  log.G(ctx).WithFields(logrus.Fields{
@@ -40,7 +40,7 @@ func NewUVMBuilder(ctx context.Context, id, owner, binPath, addr string, guestOS
  opts := &jobobject.Options{
  Name: id,
  }
- job, err := jobobject.Create(ctx, opts)
+ job, err = jobobject.Create(ctx, opts)
  if err != nil {
  return nil, errors.Wrap(err, "failed to create job object for remotevm process")
  }

pkg/cimfs/cim_test.go

@@ -87,9 +87,7 @@ func TestCimReadWrite(t *testing.T) {
  // give some time and then remove.
  time.Sleep(3 * time.Second)
  if err := DestroyCim(context.Background(), cimPath); err != nil {
- if err != nil {
- t.Fatalf("destroy cim failed: %s", err)
- }
+ t.Fatalf("destroy cim failed: %s", err)
  }
  }()
 

pkg/cimfs/cim_writer_windows.go

@@ -219,7 +219,7 @@ func DestroyCim(ctx context.Context, cimPath string) (retErr error) {
  regionFilePaths, err := getRegionFilePaths(ctx, cimPath)
  if err != nil {
  log.G(ctx).WithError(err).Warnf("get region files for cim %s", cimPath)
- if retErr == nil {
+ if retErr == nil { //nolint:govet // nilness: consistency with below
  retErr = err
  }
  }