From 63bbda08c83e42fe08a9c8b38339aeeeb93610c2 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Thu, 28 Nov 2024 21:24:50 +1100 Subject: [PATCH] errors: drop remaining errors.Wrap-like fmtcompat.Errorf users The remaining users of fmtcompat.Errorf are all something resembling err := someFunc(...) return fmtcompat.Errorf("...: %w", err) which can easily be rewritten to be more standard Go and doing explicit error checks. This removes the final users of fmtcompat.Errorf. Signed-off-by: Aleksa Sarai --- cmd/umoci/gc.go | 6 +++-- oci/casext/blob.go | 9 ++++---- oci/casext/map.go | 7 +++--- oci/layer/generate.go | 9 +++++--- oci/layer/tar_extract.go | 30 +++++++++++++++---------- oci/layer/tar_generate.go | 6 +++-- oci/layer/unpack.go | 6 +++-- pkg/unpriv/unpriv.go | 47 +++++++++++++++++++++++++++++---------- utils.go | 12 ++++++---- 9 files changed, 87 insertions(+), 45 deletions(-) diff --git a/cmd/umoci/gc.go b/cmd/umoci/gc.go index fdcac0e4..70864232 100644 --- 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 index 4efc3c57..28737497 100644 --- 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 index b6b1951c..553a7771 100644 --- 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 index 7673a013..3f189860 100644 --- 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 index da386de9..aa438ea8 100644 --- 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,15 +351,18 @@ 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 { @@ -368,8 +370,10 @@ func (te *TarExtractor) overlayFSWhiteout(dir string, file string) error { // 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 index 97e9c96d..e9077933 100644 --- 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 index 30381980..3ebc4214 100644 --- 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 index 6ef1dcbb..c7f6ac81 100644 --- a/pkg/unpriv/unpriv.go +++ b/pkg/unpriv/unpriv.go @@ -28,7 +28,6 @@ import ( "time" securejoin "github.com/cyphar/filepath-securejoin" - "github.com/opencontainers/umoci/pkg/fmtcompat" "github.com/opencontainers/umoci/pkg/system" "golang.org/x/sys/unix" ) @@ -143,7 +142,10 @@ func Open(path string) (*os.File, error) { fh, err = os.Open(path) return err }) - return fh, fmtcompat.Errorf("unpriv.open: %w", err) + if err != nil { + return nil, fmt.Errorf("unpriv.open: %w", err) + } + return fh, nil } // Create is a wrapper around os.Create which has been wrapped with unpriv.Wrap @@ -158,7 +160,10 @@ func Create(path string) (*os.File, error) { fh, err = os.Create(path) return err }) - return fh, fmtcompat.Errorf("unpriv.create: %w", err) + if err != nil { + return nil, fmt.Errorf("unpriv.create: %w", err) + } + return fh, nil } // Readdir is a wrapper around (*os.File).Readdir which has been wrapper with @@ -193,7 +198,10 @@ func Readdir(path string) ([]os.FileInfo, error) { infos, err = fh.Readdir(-1) return err }) - return infos, fmtcompat.Errorf("unpriv.readdir: %w", err) + if err != nil { + return nil, fmt.Errorf("unpriv.readdir: %w", err) + } + return infos, nil } // Lstat is a wrapper around os.Lstat which has been wrapped with unpriv.Wrap @@ -209,7 +217,10 @@ func Lstat(path string) (os.FileInfo, error) { fi, err = os.Lstat(path) return err }) - return fi, fmtcompat.Errorf("unpriv.lstat: %w", err) + if err != nil { + return nil, fmt.Errorf("unpriv.lstat: %w", err) + } + return fi, nil } // Lstatx is like Lstat but uses unix.Lstat and returns unix.Stat_t instead @@ -218,7 +229,10 @@ func Lstatx(path string) (unix.Stat_t, error) { err := Wrap(path, func(path string) error { return unix.Lstat(path, &s) }) - return s, fmtcompat.Errorf("unpriv.lstatx: %w", err) + if err != nil { + return s, fmt.Errorf("unpriv.lstatx: %w", err) + } + return s, nil } // Readlink is a wrapper around os.Readlink which has been wrapped with @@ -234,7 +248,10 @@ func Readlink(path string) (string, error) { target, err = os.Readlink(path) return err }) - return target, fmtcompat.Errorf("unpriv.readlink: %w", err) + if err != nil { + return "", fmt.Errorf("unpriv.readlink: %w", err) + } + return target, nil } // Symlink is a wrapper around os.Symlink which has been wrapped with @@ -500,7 +517,10 @@ func Llistxattr(path string) ([]string, error) { xattrs, err = system.Llistxattr(path) return err }) - return xattrs, fmtcompat.Errorf("unpriv.llistxattr: %w", err) + if err != nil { + return nil, fmt.Errorf("unpriv.llistxattr: %w", err) + } + return xattrs, nil } // Lremovexattr is a wrapper around system.Lremovexattr which has been wrapped @@ -535,7 +555,10 @@ func Lgetxattr(path, name string) ([]byte, error) { value, err = system.Lgetxattr(path, name) return err }) - return value, fmtcompat.Errorf("unpriv.lgetxattr: %w", err) + if err != nil { + return nil, fmt.Errorf("unpriv.lgetxattr: %w", err) + } + return value, nil } // Lclearxattrs is similar to system.Lclearxattrs but in order to implement it @@ -616,9 +639,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 index 441eab01..44f71785 100644 --- 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.