errors: drop remaining trivial errors.Wrap-like fmtcompat.Errorf users

The super trivial usage of return fmtcompat.Errorf("...: %w", someFunc(...)) can easily be rewritten to be more standard Go and doing explicit error checks. There are some remaining pkg/unpriv users of this pattern with unpriv.Wrap, but rewriting them would be a little ugly at the moment so we can tackle this in a future patch. Signed-off-by: Aleksa Sarai <[email protected]>
opencontainers · Nov 28, 2024 · d6122d9 · d6122d9
1 parent 48b28c9
commit d6122d9
Show file tree

Hide file tree

Showing 9 changed files with 55 additions and 36 deletions.
diff --git a/cmd/umoci/gc.go b/cmd/umoci/gc.go
@@ -24,7 +24,6 @@ import (
 
 	"github.com/opencontainers/umoci/oci/cas/dir"
 	"github.com/opencontainers/umoci/oci/casext"
-	"github.com/opencontainers/umoci/pkg/fmtcompat"
 	"github.com/urfave/cli"
 )
 
@@ -67,5 +66,8 @@ func gc(ctx *cli.Context) error {
 	defer engine.Close()
 
 	// Run the GC.
-	return fmtcompat.Errorf("gc: %w", engineExt.GC(context.Background()))
+	if err := engineExt.GC(context.Background()); err != nil {
+		return fmt.Errorf("gc: %w", err)
+	}
+	return nil
 }
diff --git a/oci/casext/blob.go b/oci/casext/blob.go
@@ -26,7 +26,6 @@ import (
 
 	ispec "github.com/opencontainers/image-spec/specs-go/v1"
 	"github.com/opencontainers/umoci/oci/casext/mediatype"
-	"github.com/opencontainers/umoci/pkg/fmtcompat"
 	"github.com/opencontainers/umoci/pkg/system"
 )
 
@@ -75,11 +74,11 @@ func (e Engine) FromDescriptor(ctx context.Context, descriptor ispec.Descriptor)
 
 	if fn := mediatype.GetParser(descriptor.MediaType); fn != nil {
 		defer func() {
-			if _, err := system.Copy(ioutil.Discard, reader); Err == nil {
-				Err = fmtcompat.Errorf("discard trailing %q blob: %w", descriptor.MediaType, err)
+			if _, err := system.Copy(ioutil.Discard, reader); Err == nil && err != nil {
+				Err = fmt.Errorf("discard trailing %q blob: %w", descriptor.MediaType, err)
 			}
-			if err := reader.Close(); Err == nil {
-				Err = fmtcompat.Errorf("close %q blob: %w", descriptor.MediaType, err)
+			if err := reader.Close(); Err == nil && err != nil {
+				Err = fmt.Errorf("close %q blob: %w", descriptor.MediaType, err)
 			}
 		}()
 

diff --git a/oci/casext/map.go b/oci/casext/map.go
@@ -24,7 +24,6 @@ import (
 	"github.com/apex/log"
 	ispec "github.com/opencontainers/image-spec/specs-go/v1"
 	"github.com/opencontainers/umoci/oci/casext/mediatype"
-	"github.com/opencontainers/umoci/pkg/fmtcompat"
 )
 
 // Used by walkState.mark() to determine which struct members are descriptors to
@@ -75,8 +74,10 @@ func mapDescriptors(V reflect.Value, mapFunc DescriptorMapFunc) error {
 		if V.IsNil() {
 			return nil
 		}
-		err := mapDescriptors(V.Elem(), mapFunc)
-		return fmtcompat.Errorf("%v: %w", V.Type(), err)
+		if err := mapDescriptors(V.Elem(), mapFunc); err != nil {
+			return fmt.Errorf("%v: %w", V.Type(), err)
+		}
+		return nil
 
 	case reflect.Slice, reflect.Array:
 		// Iterate over each element.

diff --git a/oci/layer/generate.go b/oci/layer/generate.go
@@ -26,7 +26,6 @@ import (
 	"sort"
 
 	"github.com/apex/log"
-	"github.com/opencontainers/umoci/pkg/fmtcompat"
 	"github.com/opencontainers/umoci/pkg/unpriv"
 	"github.com/vbatts/go-mtree"
 )
@@ -55,11 +54,13 @@ func GenerateLayer(path string, deltas []mtree.InodeDelta, opt *RepackOptions) (
 	go func() (Err error) {
 		// Close with the returned error.
 		defer func() {
+			var closeErr error
 			if Err != nil {
 				log.Warnf("could not generate layer: %v", Err)
+				closeErr = fmt.Errorf("generate layer: %w", Err)
 			}
 			// #nosec G104
-			_ = writer.CloseWithError(fmtcompat.Errorf("generate layer: %w", Err))
+			_ = writer.CloseWithError(closeErr)
 		}()
 
 		// We can't just dump all of the file contents into a tar file. We need
@@ -138,11 +139,13 @@ func GenerateInsertLayer(root string, target string, opaque bool, opt *RepackOpt
 
 	go func() (Err error) {
 		defer func() {
+			var closeErr error
 			if Err != nil {
 				log.Warnf("could not generate insert layer: %v", Err)
+				closeErr = fmt.Errorf("generate insert layer: %w", Err)
 			}
 			// #nosec G104
-			_ = writer.CloseWithError(fmtcompat.Errorf("generate insert layer: %w", Err))
+			_ = writer.CloseWithError(closeErr)
 		}()
 
 		tg := newTarGenerator(writer, packOptions.MapOptions)

diff --git a/oci/layer/tar_extract.go b/oci/layer/tar_extract.go
@@ -30,7 +30,6 @@ import (
 
 	"github.com/apex/log"
 	securejoin "github.com/cyphar/filepath-securejoin"
-	"github.com/opencontainers/umoci/pkg/fmtcompat"
 	"github.com/opencontainers/umoci/pkg/fseval"
 	"github.com/opencontainers/umoci/pkg/system"
 	"github.com/opencontainers/umoci/third_party/shared"
@@ -321,7 +320,7 @@ func (te *TarExtractor) ociWhiteout(root string, dir string, file string) error
 	// we iterate over any children and try again. The only difference
 	// between opaque whiteouts and regular whiteouts is that we don't
 	// delete the directory itself with opaque whiteouts.
-	err := te.fsEval.Walk(path, func(subpath string, info os.FileInfo, err error) error {
+	if err := te.fsEval.Walk(path, func(subpath string, info os.FileInfo, err error) error {
 		// If we are passed an error, bail unless it's ENOENT.
 		if err != nil {
 			// If something was deleted outside of our knowledge it's not
@@ -352,24 +351,29 @@ func (te *TarExtractor) ociWhiteout(root string, dir string, file string) error
 			// Purge the path. We skip anything underneath (if it's a
 			// directory) since we just purged it -- and we don't want to
 			// hit ENOENT during iteration for no good reason.
-			err := fmtcompat.Errorf("whiteout subpath: %w", te.fsEval.RemoveAll(subpath))
-			if err == nil && info.IsDir() {
-				err = filepath.SkipDir
+			if err := te.fsEval.RemoveAll(subpath); err != nil {
+				return fmt.Errorf("whiteout subpath: %w", err)
+			}
+			if info.IsDir() {
+				return filepath.SkipDir
 			}
-			return err
 		}
 		return nil
-	})
-	return fmtcompat.Errorf("whiteout remove: %w", err)
+	}); err != nil {
+		return fmt.Errorf("whiteout remove: %w", err)
+	}
+	return nil
 }
 
 func (te *TarExtractor) overlayFSWhiteout(dir string, file string) error {
 	isOpaque := file == whOpaque
 
 	// if this is an opaque whiteout, whiteout the directory
 	if isOpaque {
-		err := te.fsEval.Lsetxattr(dir, "trusted.overlay.opaque", []byte("y"), 0)
-		return fmtcompat.Errorf("couldn't set overlayfs whiteout attr for %s: %w", dir, err)
+		if err := te.fsEval.Lsetxattr(dir, "trusted.overlay.opaque", []byte("y"), 0); err != nil {
+			return fmt.Errorf("couldn't set overlayfs whiteout attr for %s: %w", dir, err)
+		}
+		return nil
 	}
 
 	// otherwise, white out the file itself.
@@ -378,8 +382,10 @@ func (te *TarExtractor) overlayFSWhiteout(dir string, file string) error {
 		return fmt.Errorf("couldn't create overlayfs whiteout for %s: %w", p, err)
 	}
 
-	err := te.fsEval.Mknod(p, unix.S_IFCHR|0666, unix.Mkdev(0, 0))
-	return fmtcompat.Errorf("couldn't create overlayfs whiteout for %s: %w", p, err)
+	if err := te.fsEval.Mknod(p, unix.S_IFCHR|0666, unix.Mkdev(0, 0)); err != nil {
+		return fmt.Errorf("couldn't create overlayfs whiteout for %s: %w", p, err)
+	}
+	return nil
 }
 
 // UnpackEntry extracts the given tar.Header to the provided root, ensuring

diff --git a/oci/layer/tar_generate.go b/oci/layer/tar_generate.go
@@ -27,7 +27,6 @@ import (
 	"strings"
 
 	"github.com/apex/log"
-	"github.com/opencontainers/umoci/pkg/fmtcompat"
 	"github.com/opencontainers/umoci/pkg/fseval"
 	"github.com/opencontainers/umoci/pkg/system"
 	"github.com/opencontainers/umoci/pkg/testutils"
@@ -315,7 +314,10 @@ func (tg *tarGenerator) addWhiteout(name string, opaque bool) error {
 	}
 
 	// Add a dummy header for the whiteout file.
-	return fmtcompat.Errorf("write whiteout header: %w", tg.tw.WriteHeader(&tar.Header{Name: whiteout, Size: 0}))
+	if err := tg.tw.WriteHeader(&tar.Header{Name: whiteout, Size: 0}); err != nil {
+		return fmt.Errorf("write whiteout header: %w", err)
+	}
+	return nil
 }
 
 // AddWhiteout creates a whiteout for the provided path.

diff --git a/oci/layer/unpack.go b/oci/layer/unpack.go
@@ -40,7 +40,6 @@ import (
 	"github.com/opencontainers/umoci/oci/cas"
 	"github.com/opencontainers/umoci/oci/casext"
 	iconv "github.com/opencontainers/umoci/oci/config/convert"
-	"github.com/opencontainers/umoci/pkg/fmtcompat"
 	"github.com/opencontainers/umoci/pkg/fseval"
 	"github.com/opencontainers/umoci/pkg/idtools"
 	"github.com/opencontainers/umoci/pkg/system"
@@ -374,5 +373,8 @@ func UnpackRuntimeJSON(ctx context.Context, engine cas.Engine, configFile io.Wri
 	// Save the config.json.
 	enc := json.NewEncoder(configFile)
 	enc.SetIndent("", "\t")
-	return fmtcompat.Errorf("write config.json: %w", enc.Encode(spec))
+	if err := enc.Encode(spec); err != nil {
+		return fmt.Errorf("write config.json: %w", err)
+	}
+	return nil
 }
diff --git a/pkg/unpriv/unpriv.go b/pkg/unpriv/unpriv.go
@@ -616,9 +616,9 @@ func Walk(root string, walkFn filepath.WalkFunc) error {
 		} else {
 			err = walk(root, info, walkFn)
 		}
-		if !errors.Is(err, filepath.SkipDir) {
-			return fmtcompat.Errorf("unpriv.walk: %w", err)
+		if err == nil || errors.Is(err, filepath.SkipDir) {
+			return nil
 		}
-		return nil
+		return fmt.Errorf("unpriv.walk: %w", err)
 	})
 }
diff --git a/utils.go b/utils.go
@@ -34,7 +34,6 @@ import (
 	"github.com/opencontainers/umoci/oci/casext"
 	igen "github.com/opencontainers/umoci/oci/config/generate"
 	"github.com/opencontainers/umoci/oci/layer"
-	"github.com/opencontainers/umoci/pkg/fmtcompat"
 	"github.com/opencontainers/umoci/pkg/idtools"
 	"github.com/urfave/cli"
 	"github.com/vbatts/go-mtree"
@@ -110,8 +109,10 @@ func WriteBundleMeta(bundle string, meta Meta) error {
 	}
 	defer fh.Close()
 
-	_, err = meta.WriteTo(fh)
-	return fmtcompat.Errorf("write metadata: %w", err)
+	if _, err := meta.WriteTo(fh); err != nil {
+		return fmt.Errorf("write metadata: %w", err)
+	}
+	return nil
 }
 
 // ReadBundleMeta reads and parses the umoci.json file from a given bundle path.
@@ -130,7 +131,10 @@ func ReadBundleMeta(bundle string) (Meta, error) {
 			err = fmt.Errorf("unsupported umoci.json version: %s", meta.Version)
 		}
 	}
-	return meta, fmtcompat.Errorf("decode metadata: %w", err)
+	if err != nil {
+		return meta, fmt.Errorf("decode metadata: %w", err)
+	}
+	return meta, nil
 }
 
 // ManifestStat has information about a given OCI manifest.