Skip to content

Commit

Permalink
errors: drop fmtcompat.Errorf for complicated err != nil checks
Browse files Browse the repository at this point in the history
These are more complicated checks than the trivial

  if err != nil {
      return fmt.Errorf("...: %w", err)
  }

but are similar and can be ported for the same reasons as the simple
case, except they needed to be hand-checked for correctness.

Signed-off-by: Aleksa Sarai <[email protected]>
  • Loading branch information
cyphar committed Nov 28, 2024
1 parent 2a5d62d commit 3587052
Show file tree
Hide file tree
Showing 15 changed files with 51 additions and 60 deletions.
3 changes: 1 addition & 2 deletions cmd/umoci/init.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import (

"github.com/apex/log"
"github.com/opencontainers/umoci/oci/cas/dir"
"github.com/opencontainers/umoci/pkg/fmtcompat"
"github.com/urfave/cli"
)

Expand Down Expand Up @@ -59,7 +58,7 @@ func initLayout(ctx *cli.Context) error {
if err == nil {
err = fmt.Errorf("path already exists: %s", imagePath)
}
return fmtcompat.Errorf("image layout creation: %w", err)
return fmt.Errorf("image layout creation: %w", err)
}

if err := dir.Create(imagePath); err != nil {
Expand Down
3 changes: 1 addition & 2 deletions cmd/umoci/insert.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,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/urfave/cli"
)

Expand Down Expand Up @@ -192,7 +191,7 @@ func insert(ctx *cli.Context) error {
// TODO: We should add a flag to allow for a new layer to be made
// non-distributable.
if _, err := mutator.Add(context.Background(), ispec.MediaTypeImageLayer, reader, history, mutate.GzipCompressor, nil); err != nil {
return fmtcompat.Errorf("add diff layer: %w", err)
return fmt.Errorf("add diff layer: %w", err)
}

newDescriptorPath, err := mutator.Commit(context.Background())
Expand Down
5 changes: 2 additions & 3 deletions cmd/umoci/raw-add-layer.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ import (
"github.com/opencontainers/umoci/oci/cas/dir"
"github.com/opencontainers/umoci/oci/casext"
igen "github.com/opencontainers/umoci/oci/config/generate"
"github.com/opencontainers/umoci/pkg/fmtcompat"
"github.com/urfave/cli"
)

Expand Down Expand Up @@ -114,7 +113,7 @@ func rawAddLayer(ctx *cli.Context) error {
return fmt.Errorf("open new layer archive: %w", err)
}
if fi, err := newLayer.Stat(); err != nil {
return fmtcompat.Errorf("stat new layer archive: %w", err)
return fmt.Errorf("stat new layer archive: %w", err)
} else if fi.IsDir() {
return errors.New("new layer archive is a directory")
}
Expand Down Expand Up @@ -158,7 +157,7 @@ func rawAddLayer(ctx *cli.Context) error {
// TODO: We should add a flag to allow for a new layer to be made
// non-distributable.
if _, err := mutator.Add(context.Background(), ispec.MediaTypeImageLayer, newLayer, history, mutate.GzipCompressor, nil); err != nil {
return fmtcompat.Errorf("add diff layer: %w", err)
return fmt.Errorf("add diff layer: %w", err)
}

newDescriptorPath, err := mutator.Commit(context.Background())
Expand Down
9 changes: 4 additions & 5 deletions mutate/compress.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
"github.com/apex/log"
zstd "github.com/klauspost/compress/zstd"
gzip "github.com/klauspost/pgzip"
"github.com/opencontainers/umoci/pkg/fmtcompat"
"github.com/opencontainers/umoci/pkg/system"
)

Expand Down Expand Up @@ -65,14 +64,14 @@ func (gz *gzipCompressor) Compress(reader io.Reader) (io.ReadCloser, error) {
if err != nil {
log.Warnf("gzip compress: could not compress layer: %v", err)
// #nosec G104
_ = pipeWriter.CloseWithError(fmtcompat.Errorf("compressing layer: %w", err))
_ = pipeWriter.CloseWithError(fmt.Errorf("compressing layer: %w", err))
return
}
gz.bytesRead = bytesRead
if err := gzw.Close(); err != nil {
log.Warnf("gzip compress: could not close gzip writer: %v", err)
// #nosec G104
_ = pipeWriter.CloseWithError(fmtcompat.Errorf("close gzip writer: %w", err))
_ = pipeWriter.CloseWithError(fmt.Errorf("close gzip writer: %w", err))
return
}
if err := pipeWriter.Close(); err != nil {
Expand Down Expand Up @@ -112,14 +111,14 @@ func (zs *zstdCompressor) Compress(reader io.Reader) (io.ReadCloser, error) {
if err != nil {
log.Warnf("zstd compress: could not compress layer: %v", err)
// #nosec G104
_ = pipeWriter.CloseWithError(fmtcompat.Errorf("compressing layer: %w", err))
_ = pipeWriter.CloseWithError(fmt.Errorf("compressing layer: %w", err))
return
}
zs.bytesRead = bytesRead
if err := zw.Close(); err != nil {
log.Warnf("zstd compress: could not close gzip writer: %v", err)
// #nosec G104
_ = pipeWriter.CloseWithError(fmtcompat.Errorf("close zstd writer: %w", err))
_ = pipeWriter.CloseWithError(fmt.Errorf("close zstd writer: %w", err))
return
}
if err := pipeWriter.Close(); err != nil {
Expand Down
19 changes: 9 additions & 10 deletions oci/cas/dir/dir.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ import (
imeta "github.com/opencontainers/image-spec/specs-go"
ispec "github.com/opencontainers/image-spec/specs-go/v1"
"github.com/opencontainers/umoci/oci/cas"
"github.com/opencontainers/umoci/pkg/fmtcompat"
"github.com/opencontainers/umoci/pkg/hardening"
"github.com/opencontainers/umoci/pkg/system"
"golang.org/x/sys/unix"
Expand Down Expand Up @@ -111,7 +110,7 @@ func (e *dirEngine) validate() error {
if errors.Is(err, os.ErrNotExist) {
err = cas.ErrInvalid
}
return fmtcompat.Errorf("read oci-layout: %w", err)
return fmt.Errorf("read oci-layout: %w", err)
}

var ociLayout ispec.ImageLayout
Expand All @@ -122,7 +121,7 @@ func (e *dirEngine) validate() error {
// XXX: Currently the meaning of this field is not adequately defined by
// the spec, nor is the "official" value determined by the spec.
if ociLayout.Version != ImageLayoutVersion {
return fmtcompat.Errorf("layout version is not supported: %w", cas.ErrInvalid)
return fmt.Errorf("layout version is not supported: %w", cas.ErrInvalid)
}

// Check that "blobs" and "index.json" exist in the image.
Expand All @@ -133,18 +132,18 @@ func (e *dirEngine) validate() error {
if errors.Is(err, os.ErrNotExist) {
err = cas.ErrInvalid
}
return fmtcompat.Errorf("check blobdir: %w", err)
return fmt.Errorf("check blobdir: %w", err)
} else if !fi.IsDir() {
return fmtcompat.Errorf("blobdir is not a directory: %w", cas.ErrInvalid)
return fmt.Errorf("blobdir is not a directory: %w", cas.ErrInvalid)
}

if fi, err := os.Stat(filepath.Join(e.path, indexFile)); err != nil {
if errors.Is(err, os.ErrNotExist) {
err = cas.ErrInvalid
}
return fmtcompat.Errorf("check index: %w", err)
return fmt.Errorf("check index: %w", err)
} else if fi.IsDir() {
return fmtcompat.Errorf("index is a directory: %w", cas.ErrInvalid)
return fmt.Errorf("index is a directory: %w", cas.ErrInvalid)
}

return nil
Expand Down Expand Up @@ -292,7 +291,7 @@ func (e *dirEngine) GetIndex(ctx context.Context) (ispec.Index, error) {
if errors.Is(err, os.ErrNotExist) {
err = cas.ErrInvalid
}
return ispec.Index{}, fmtcompat.Errorf("read index: %w", err)
return ispec.Index{}, fmt.Errorf("read index: %w", err)
}

var index ispec.Index
Expand All @@ -314,7 +313,7 @@ func (e *dirEngine) DeleteBlob(ctx context.Context, digest digest.Digest) error

err = os.Remove(filepath.Join(e.path, path))
if err != nil && !errors.Is(err, os.ErrNotExist) {
return fmtcompat.Errorf("remove blob: %w", err)
return fmt.Errorf("remove blob: %w", err)
}
return nil
}
Expand Down Expand Up @@ -362,7 +361,7 @@ func (e *dirEngine) Clean(ctx context.Context) error {
func (e *dirEngine) cleanPath(ctx context.Context, path string) error {
cfh, err := os.Open(path)
if err != nil && !errors.Is(err, os.ErrNotExist) {
return fmtcompat.Errorf("open for locking: %w", err)
return fmt.Errorf("open for locking: %w", err)
}
defer cfh.Close()

Expand Down
3 changes: 1 addition & 2 deletions oci/config/convert/runtime.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import (
ispec "github.com/opencontainers/image-spec/specs-go/v1"
rspec "github.com/opencontainers/runtime-spec/specs-go"
igen "github.com/opencontainers/umoci/oci/config/generate"
"github.com/opencontainers/umoci/pkg/fmtcompat"
)

// Annotations described by the OCI image-spec document (these represent fields
Expand Down Expand Up @@ -186,7 +185,7 @@ func MutateRuntimeSpec(spec *rspec.Spec, rootfs string, image ispec.Image) error
// We only log an error if were not given a rootfs, and we set execUser
// to the "default" (root:root).
if rootfs != "" {
return fmtcompat.Errorf("cannot parse user spec: %q: %w", ig.ConfigUser(), err)
return fmt.Errorf("cannot parse user spec: %q: %w", ig.ConfigUser(), err)
}
log.Warnf("could not parse user spec %q without a rootfs -- defaulting to root:root", ig.ConfigUser())
execUser = new(user.ExecUser)
Expand Down
6 changes: 3 additions & 3 deletions oci/layer/generate.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,19 +102,19 @@ func GenerateLayer(path string, deltas []mtree.InodeDelta, opt *RepackOptions) (
}
if err := tg.AddFile(name, fullPath); err != nil {
log.Warnf("generate layer: could not add file %q: %s", name, err)
return fmtcompat.Errorf("generate layer file: %w", err)
return fmt.Errorf("generate layer file: %w", err)
}
case mtree.Missing:
if err := tg.AddWhiteout(name); err != nil {
log.Warnf("generate layer: could not add whiteout %q: %s", name, err)
return fmtcompat.Errorf("generate whiteout layer file: %w", err)
return fmt.Errorf("generate whiteout layer file: %w", err)
}
}
}

if err := tg.tw.Close(); err != nil {
log.Warnf("generate layer: could not close tar.Writer: %s", err)
return fmtcompat.Errorf("close tar writer: %w", err)
return fmt.Errorf("close tar writer: %w", err)
}

return nil
Expand Down
18 changes: 9 additions & 9 deletions oci/layer/tar_extract.go
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ func (te *TarExtractor) restoreMetadata(path string, hdr *tar.Header) error {
err := te.fsEval.Lclearxattrs(path, ignoreXattrs)
if err != nil {
if !errors.Is(err, unix.ENOTSUP) {
return fmtcompat.Errorf("clear xattr metadata: %s: %w", path, err)
return fmt.Errorf("clear xattr metadata: %s: %w", path, err)
}
if !te.enotsupWarned {
log.Warnf("xattr{%s} ignoring ENOTSUP on clearxattrs", path)
Expand Down Expand Up @@ -210,7 +210,7 @@ func (te *TarExtractor) restoreMetadata(path string, hdr *tar.Header) error {
}
continue
}
return fmtcompat.Errorf("restore xattr metadata: %s: %w", path, err)
return fmt.Errorf("restore xattr metadata: %s: %w", path, err)
}
}

Expand Down Expand Up @@ -244,7 +244,7 @@ func (te *TarExtractor) applyMetadata(path string, hdr *tar.Header) error {
func (te *TarExtractor) isDirlink(root string, path string) (bool, error) {
// Make sure it exists and is a symlink.
if _, err := te.fsEval.Readlink(path); err != nil {
return false, fmtcompat.Errorf("read dirlink: %w", err)
return false, fmt.Errorf("read dirlink: %w", err)
}

// Technically a string.TrimPrefix would also work...
Expand All @@ -264,7 +264,7 @@ func (te *TarExtractor) isDirlink(root string, path string) (bool, error) {
if errors.Is(err, unix.ELOOP) {
return false, nil
}
return false, fmtcompat.Errorf("sanitize old target: %w", err)
return false, fmt.Errorf("sanitize old target: %w", err)
}

targetInfo, err := te.fsEval.Lstat(targetPath)
Expand Down Expand Up @@ -312,7 +312,7 @@ func (te *TarExtractor) ociWhiteout(root string, dir string, file string) error
if securejoin.IsNotExist(err) {
return nil
}
return fmtcompat.Errorf("check whiteout target: %w", err)
return fmt.Errorf("check whiteout target: %w", err)
}

// Walk over the path to remove it. We remove a given path as soon as
Expand Down Expand Up @@ -375,7 +375,7 @@ func (te *TarExtractor) overlayFSWhiteout(dir string, file string) error {
// otherwise, white out the file itself.
p := filepath.Join(dir, strings.TrimPrefix(file, whPrefix))
if err := os.RemoveAll(p); err != nil && !errors.Is(err, os.ErrNotExist) {
return fmtcompat.Errorf("couldn't create overlayfs whiteout for %s: %w", p, err)
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))
Expand Down Expand Up @@ -445,7 +445,7 @@ func (te *TarExtractor) UnpackEntry(root string, hdr *tar.Header, r io.Reader) (
xattrs, err := te.fsEval.Llistxattr(dir)
if err != nil {
if !errors.Is(err, unix.ENOTSUP) {
return fmtcompat.Errorf("get dirHdr.Xattrs: %w", err)
return fmt.Errorf("get dirHdr.Xattrs: %w", err)
}
if !te.enotsupWarned {
log.Warnf("xattr{%s} ignoring ENOTSUP on llistxattr", dir)
Expand Down Expand Up @@ -473,7 +473,7 @@ func (te *TarExtractor) UnpackEntry(root string, hdr *tar.Header, r io.Reader) (
// Only overwrite the error if there wasn't one already.
if err := te.restoreMetadata(dir, dirHdr); err != nil {
if Err == nil {
Err = fmtcompat.Errorf("restore parent directory: %w", err)
Err = fmt.Errorf("restore parent directory: %w", err)
}
}
}()
Expand Down Expand Up @@ -577,7 +577,7 @@ func (te *TarExtractor) UnpackEntry(root string, hdr *tar.Header, r io.Reader) (
n, err := system.Copy(fh, r)
if int64(n) != hdr.Size {
if err != nil {
err = fmtcompat.Errorf("short write: %w", err)
err = fmt.Errorf("short write: %w", err)
} else {
err = io.ErrShortWrite
}
Expand Down
8 changes: 4 additions & 4 deletions oci/layer/tar_generate.go
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ func (tg *tarGenerator) AddFile(name, path string) error {
linkname := ""
if fi.Mode()&os.ModeSymlink == os.ModeSymlink {
if linkname, err = tg.fsEval.Readlink(path); err != nil {
return fmtcompat.Errorf("add file readlink: %w", err)
return fmt.Errorf("add file readlink: %w", err)
}
}

Expand Down Expand Up @@ -205,7 +205,7 @@ func (tg *tarGenerator) AddFile(name, path string) error {
names, err := tg.fsEval.Llistxattr(path)
if err != nil {
if !errors.Is(err, unix.EOPNOTSUPP) {
return fmtcompat.Errorf("get xattr list: %w", err)
return fmt.Errorf("get xattr list: %w", err)
}
names = []string{}
}
Expand All @@ -227,7 +227,7 @@ func (tg *tarGenerator) AddFile(name, path string) error {
// XXX: I'm not sure if we're unprivileged whether Lgetxattr can
// fail with EPERM. If it can, we should ignore it (like when
// we try to clear xattrs).
return fmtcompat.Errorf("get xattr: %s: %w", name, err)
return fmt.Errorf("get xattr: %s: %w", name, err)
}
}
// https://golang.org/issues/20698 -- We don't just error out here
Expand Down Expand Up @@ -276,7 +276,7 @@ func (tg *tarGenerator) AddFile(name, path string) error {
return fmt.Errorf("copy to layer: %w", err)
}
if n != hdr.Size {
return fmtcompat.Errorf("copy to layer: %w", io.ErrShortWrite)
return fmt.Errorf("copy to layer: %w", io.ErrShortWrite)
}
}

Expand Down
10 changes: 5 additions & 5 deletions oci/layer/unpack.go
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ func UnpackManifest(ctx context.Context, engine cas.Engine, bundle string, manif
if err == nil {
return fmt.Errorf("config.json already exists in %s", bundle)
}
return fmtcompat.Errorf("problem accessing bundle config: %w", err)
return fmt.Errorf("problem accessing bundle config: %w", err)
}

defer func() {
Expand All @@ -136,7 +136,7 @@ func UnpackManifest(ctx context.Context, engine cas.Engine, bundle string, manif
if err == nil {
err = fmt.Errorf("%s already exists", rootfsPath)
}
return fmtcompat.Errorf("detecting rootfs: %w", err)
return fmt.Errorf("detecting rootfs: %w", err)
}

log.Infof("unpack rootfs: %s", rootfsPath)
Expand All @@ -163,7 +163,7 @@ func UnpackRootfs(ctx context.Context, engine cas.Engine, rootfsPath string, man
engineExt := casext.NewEngine(engine)

if err := os.Mkdir(rootfsPath, 0755); err != nil && !os.IsExist(err) {
return fmtcompat.Errorf("mkdir rootfs: %w", err)
return fmt.Errorf("mkdir rootfs: %w", err)
}

// In order to avoid having a broken rootfs in the case of an error, we
Expand Down Expand Up @@ -276,7 +276,7 @@ func UnpackRootfs(ctx context.Context, engine cas.Engine, rootfsPath string, man
// the whole uncompressed stream). Just blindly consume anything left
// in the layer.
if n, err := system.Copy(ioutil.Discard, layer); err != nil {
return fmtcompat.Errorf("discard trailing archive bits: %w", err)
return fmt.Errorf("discard trailing archive bits: %w", err)
} else if n != 0 {
log.Debugf("unpack manifest: layer %s: ignoring %d trailing 'junk' bytes in the tar stream -- probably from GNU tar", layerDescriptor.Digest, n)
}
Expand All @@ -288,7 +288,7 @@ func UnpackRootfs(ctx context.Context, engine cas.Engine, rootfsPath string, man
// WriteTo, which causes havoc with system.Copy. Ideally we would use
// layerRaw. See <https://github.com/klauspost/pgzip/issues/38>.
if n, err := system.Copy(ioutil.Discard, layerData); err != nil {
return fmtcompat.Errorf("discard trailing raw bits: %w", err)
return fmt.Errorf("discard trailing raw bits: %w", err)
} else if n != 0 {
log.Warnf("unpack manifest: layer %s: ignoring %d trailing 'junk' bytes in the blob stream -- this may indicate a bug in the tool which built this image", layerDescriptor.Digest, n)
}
Expand Down
Loading

0 comments on commit 3587052

Please sign in to comment.