Skip to content

Commit

Permalink
Switch to golang native error wrapping
Browse files Browse the repository at this point in the history
We now use the golang error wrapping format specifier `%w` instead of the
deprecated github.com/pkg/errors package.

Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
  • Loading branch information
saschagrunert committed Jul 7, 2022
1 parent 20281e0 commit 3455d12
Show file tree
Hide file tree
Showing 42 changed files with 338 additions and 345 deletions.
13 changes: 6 additions & 7 deletions containers.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package storage

import (
"errors"
"fmt"
"io/ioutil"
"os"
Expand All @@ -13,7 +14,6 @@ import (
"github.com/containers/storage/pkg/stringid"
"github.com/containers/storage/pkg/truncindex"
digest "github.com/opencontainers/go-digest"
"github.com/pkg/errors"
)

// A Container is a reference to a read-write layer with metadata.
Expand Down Expand Up @@ -329,8 +329,7 @@ func (r *containerStore) Create(id string, names []string, image, layer, metadat
names = dedupeNames(names)
for _, name := range names {
if _, nameInUse := r.byname[name]; nameInUse {
return nil, errors.Wrapf(ErrDuplicateName,
fmt.Sprintf("the container name \"%s\" is already in use by \"%s\". You have to remove that container to be able to reuse that name.", name, r.byname[name].ID))
return nil, fmt.Errorf("the container name %q is already in use by %s. You have to remove that container to be able to reuse that name: %w", name, r.byname[name].ID, ErrDuplicateName)
}
}
if err := hasOverlappingRanges(options.UIDMap); err != nil {
Expand Down Expand Up @@ -479,7 +478,7 @@ func (r *containerStore) Exists(id string) bool {

func (r *containerStore) BigData(id, key string) ([]byte, error) {
if key == "" {
return nil, errors.Wrapf(ErrInvalidBigDataName, "can't retrieve container big data value for empty name")
return nil, fmt.Errorf("can't retrieve container big data value for empty name: %w", ErrInvalidBigDataName)
}
c, ok := r.lookup(id)
if !ok {
Expand All @@ -490,7 +489,7 @@ func (r *containerStore) BigData(id, key string) ([]byte, error) {

func (r *containerStore) BigDataSize(id, key string) (int64, error) {
if key == "" {
return -1, errors.Wrapf(ErrInvalidBigDataName, "can't retrieve size of container big data with empty name")
return -1, fmt.Errorf("can't retrieve size of container big data with empty name: %w", ErrInvalidBigDataName)
}
c, ok := r.lookup(id)
if !ok {
Expand Down Expand Up @@ -520,7 +519,7 @@ func (r *containerStore) BigDataSize(id, key string) (int64, error) {

func (r *containerStore) BigDataDigest(id, key string) (digest.Digest, error) {
if key == "" {
return "", errors.Wrapf(ErrInvalidBigDataName, "can't retrieve digest of container big data value with empty name")
return "", fmt.Errorf("can't retrieve digest of container big data value with empty name: %w", ErrInvalidBigDataName)
}
c, ok := r.lookup(id)
if !ok {
Expand Down Expand Up @@ -558,7 +557,7 @@ func (r *containerStore) BigDataNames(id string) ([]string, error) {

func (r *containerStore) SetBigData(id, key string, data []byte) error {
if key == "" {
return errors.Wrapf(ErrInvalidBigDataName, "can't set empty name for container big data item")
return fmt.Errorf("can't set empty name for container big data item: %w", ErrInvalidBigDataName)
}
c, ok := r.lookup(id)
if !ok {
Expand Down
22 changes: 11 additions & 11 deletions drivers/aufs/aufs.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ package aufs

import (
"bufio"
"errors"
"fmt"
"io"
"io/fs"
Expand All @@ -48,7 +49,6 @@ import (
"github.com/containers/storage/pkg/system"
"github.com/opencontainers/runc/libcontainer/userns"
"github.com/opencontainers/selinux/go-selinux/label"
"github.com/pkg/errors"
"github.com/sirupsen/logrus"
"github.com/vbatts/tar-split/tar/storage"
"golang.org/x/sys/unix"
Expand Down Expand Up @@ -91,7 +91,7 @@ func Init(home string, options graphdriver.Options) (graphdriver.Driver, error)

// Try to load the aufs kernel module
if err := supportsAufs(); err != nil {
return nil, errors.Wrap(graphdriver.ErrNotSupported, "kernel does not support aufs")
return nil, fmt.Errorf("kernel does not support aufs: %w", graphdriver.ErrNotSupported)

}

Expand All @@ -106,7 +106,7 @@ func Init(home string, options graphdriver.Options) (graphdriver.Driver, error)
switch fsMagic {
case graphdriver.FsMagicAufs, graphdriver.FsMagicBtrfs, graphdriver.FsMagicEcryptfs:
logrus.Errorf("AUFS is not supported over %s", backingFs)
return nil, errors.Wrapf(graphdriver.ErrIncompatibleFS, "AUFS is not supported over %q", backingFs)
return nil, fmt.Errorf("AUFS is not supported over %q: %w", backingFs, graphdriver.ErrIncompatibleFS)
}

var mountOptions string
Expand Down Expand Up @@ -374,10 +374,10 @@ func (a *Driver) Remove(id string) error {
}

if err != unix.EBUSY {
return errors.Wrapf(err, "aufs: unmount error: %s", mountpoint)
return fmt.Errorf("aufs: unmount error: %s: %w", mountpoint, err)
}
if retries >= 5 {
return errors.Wrapf(err, "aufs: unmount error after retries: %s", mountpoint)
return fmt.Errorf("aufs: unmount error after retries: %s: %w", mountpoint, err)
}
// If unmount returns EBUSY, it could be a transient error. Sleep and retry.
retries++
Expand All @@ -387,21 +387,21 @@ func (a *Driver) Remove(id string) error {

// Remove the layers file for the id
if err := os.Remove(path.Join(a.rootPath(), "layers", id)); err != nil && !os.IsNotExist(err) {
return errors.Wrapf(err, "error removing layers dir for %s", id)
return fmt.Errorf("error removing layers dir for %s: %w", id, err)
}

if err := atomicRemove(a.getDiffPath(id)); err != nil {
return errors.Wrapf(err, "could not remove diff path for id %s", id)
return fmt.Errorf("could not remove diff path for id %s: %w", id, err)
}

// Atomically remove each directory in turn by first moving it out of the
// way (so that container runtime doesn't find it anymore) before doing removal of
// the whole tree.
if err := atomicRemove(mountpoint); err != nil {
if errors.Cause(err) == unix.EBUSY {
if errors.Is(err, unix.EBUSY) {
logger.WithField("dir", mountpoint).WithError(err).Warn("error performing atomic remove due to EBUSY")
}
return errors.Wrapf(err, "could not remove mountpoint for id %s", id)
return fmt.Errorf("could not remove mountpoint for id %s: %w", id, err)
}

a.pathCacheLock.Lock()
Expand All @@ -419,10 +419,10 @@ func atomicRemove(source string) error {
case os.IsExist(err):
// Got error saying the target dir already exists, maybe the source doesn't exist due to a previous (failed) remove
if _, e := os.Stat(source); !os.IsNotExist(e) {
return errors.Wrapf(err, "target rename dir '%s' exists but should not, this needs to be manually cleaned up", target)
return fmt.Errorf("target rename dir '%s' exists but should not, this needs to be manually cleaned up: %w", target, err)
}
default:
return errors.Wrapf(err, "error preparing atomic delete")
return fmt.Errorf("error preparing atomic delete: %w", err)
}

return system.EnsureRemoveAll(target)
Expand Down
5 changes: 3 additions & 2 deletions drivers/aufs/aufs_test.go
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
//go:build linux
// +build linux

package aufs

import (
"crypto/sha256"
"encoding/hex"
"errors"
"fmt"
"io/ioutil"
"os"
Expand All @@ -18,7 +20,6 @@ import (
"github.com/containers/storage/pkg/archive"
"github.com/containers/storage/pkg/reexec"
"github.com/containers/storage/pkg/stringid"
"github.com/pkg/errors"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
Expand All @@ -35,7 +36,7 @@ func init() {
func testInit(dir string, t testing.TB) graphdriver.Driver {
d, err := Init(dir, graphdriver.Options{})
if err != nil {
if errors.Cause(err) == graphdriver.ErrNotSupported {
if errors.Is(err, graphdriver.ErrNotSupported) {
t.Skip(err)
} else {
t.Fatal(err)
Expand Down
3 changes: 1 addition & 2 deletions drivers/btrfs/btrfs.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ import (
"github.com/containers/storage/pkg/system"
"github.com/docker/go-units"
"github.com/opencontainers/selinux/go-selinux/label"
"github.com/pkg/errors"
"github.com/sirupsen/logrus"
"golang.org/x/sys/unix"
)
Expand All @@ -62,7 +61,7 @@ func Init(home string, options graphdriver.Options) (graphdriver.Driver, error)
}

if fsMagic != graphdriver.FsMagicBtrfs {
return nil, errors.Wrapf(graphdriver.ErrPrerequisites, "%q is not on a btrfs filesystem", home)
return nil, fmt.Errorf("%q is not on a btrfs filesystem: %w", home, graphdriver.ErrPrerequisites)
}

rootUID, rootGID, err := idtools.GetRootUIDGID(options.UIDMaps, options.GIDMaps)
Expand Down
41 changes: 21 additions & 20 deletions drivers/devmapper/device_setup.go
Original file line number Diff line number Diff line change
@@ -1,18 +1,19 @@
//go:build linux && cgo
// +build linux,cgo

package devmapper

import (
"bufio"
"bytes"
"errors"
"fmt"
"io/ioutil"
"os"
"os/exec"
"path/filepath"
"strings"

"github.com/pkg/errors"
"github.com/sirupsen/logrus"
)

Expand Down Expand Up @@ -59,7 +60,7 @@ func checkDevAvailable(dev string) error {
}

if !bytes.Contains(out, []byte(dev)) {
return errors.Errorf("%s is not available for use with devicemapper", dev)
return fmt.Errorf("%s is not available for use with devicemapper", dev)
}
return nil
}
Expand All @@ -84,7 +85,7 @@ func checkDevInVG(dev string) error {
// got "VG Name" line"
vg := strings.TrimSpace(fields[1])
if len(vg) > 0 {
return errors.Errorf("%s is already part of a volume group %q: must remove this device from any volume group or provide a different device", dev, vg)
return fmt.Errorf("%s is already part of a volume group %q: must remove this device from any volume group or provide a different device", dev, vg)
}
logrus.Error(fields)
break
Expand Down Expand Up @@ -112,7 +113,7 @@ func checkDevHasFS(dev string) error {
if bytes.Equal(kv[0], []byte("TYPE")) {
v := bytes.Trim(kv[1], "\"")
if len(v) > 0 {
return errors.Errorf("%s has a filesystem already, use dm.directlvm_device_force=true if you want to wipe the device", dev)
return fmt.Errorf("%s has a filesystem already, use dm.directlvm_device_force=true if you want to wipe the device", dev)
}
return nil
}
Expand All @@ -123,16 +124,16 @@ func checkDevHasFS(dev string) error {
func verifyBlockDevice(dev string, force bool) error {
absPath, err := filepath.Abs(dev)
if err != nil {
return errors.Errorf("unable to get absolute path for %s: %s", dev, err)
return fmt.Errorf("unable to get absolute path for %s: %s", dev, err)
}
realPath, err := filepath.EvalSymlinks(absPath)
if err != nil {
return errors.Errorf("failed to canonicalise path for %s: %s", dev, err)
return fmt.Errorf("failed to canonicalise path for %s: %s", dev, err)
}
if err := checkDevAvailable(absPath); err != nil {
logrus.Infof("block device '%s' not available, checking '%s'", absPath, realPath)
if err := checkDevAvailable(realPath); err != nil {
return errors.Errorf("neither '%s' nor '%s' are in the output of lvmdiskscan, can't use device.", absPath, realPath)
return fmt.Errorf("neither '%s' nor '%s' are in the output of lvmdiskscan, can't use device", absPath, realPath)
}
}
if err := checkDevInVG(realPath); err != nil {
Expand All @@ -158,7 +159,7 @@ func readLVMConfig(root string) (directLVMConfig, error) {
if os.IsNotExist(err) {
return cfg, nil
}
return cfg, errors.Wrap(err, "error reading existing setup config")
return cfg, fmt.Errorf("error reading existing setup config: %w", err)
}

// check if this is just an empty file, no need to produce a json error later if so
Expand All @@ -167,17 +168,17 @@ func readLVMConfig(root string) (directLVMConfig, error) {
}

err = json.Unmarshal(b, &cfg)
return cfg, errors.Wrap(err, "error unmarshaling previous device setup config")
return cfg, fmt.Errorf("error unmarshaling previous device setup config: %w", err)
}

func writeLVMConfig(root string, cfg directLVMConfig) error {
p := filepath.Join(root, "setup-config.json")
b, err := json.Marshal(cfg)
if err != nil {
return errors.Wrap(err, "error marshalling direct lvm config")
return fmt.Errorf("error marshalling direct lvm config: %w", err)
}
err = ioutil.WriteFile(p, b, 0600)
return errors.Wrap(err, "error writing direct lvm config to file")
return fmt.Errorf("error writing direct lvm config to file: %w", err)
}

func setupDirectLVM(cfg directLVMConfig) error {
Expand All @@ -186,13 +187,13 @@ func setupDirectLVM(cfg directLVMConfig) error {

for _, bin := range binaries {
if _, err := exec.LookPath(bin); err != nil {
return errors.Wrap(err, "error looking up command `"+bin+"` while setting up direct lvm")
return fmt.Errorf("error looking up command `"+bin+"` while setting up direct lvm: %w", err)
}
}

err := os.MkdirAll(lvmProfileDir, 0755)
if err != nil {
return errors.Wrap(err, "error creating lvm profile directory")
return fmt.Errorf("error creating lvm profile directory: %w", err)
}

if cfg.AutoExtendPercent == 0 {
Expand All @@ -215,34 +216,34 @@ func setupDirectLVM(cfg directLVMConfig) error {

out, err := exec.Command("pvcreate", "--metadatasize", cfg.MetaDataSize, "-f", cfg.Device).CombinedOutput()
if err != nil {
return errors.Wrap(err, string(out))
return fmt.Errorf("%v: %w", string(out), err)
}

out, err = exec.Command("vgcreate", "storage", cfg.Device).CombinedOutput()
if err != nil {
return errors.Wrap(err, string(out))
return fmt.Errorf("%v: %w", string(out), err)
}

out, err = exec.Command("lvcreate", "--wipesignatures", "y", "-n", "thinpool", "storage", "--extents", fmt.Sprintf("%d%%VG", cfg.ThinpPercent)).CombinedOutput()
if err != nil {
return errors.Wrap(err, string(out))
return fmt.Errorf("%v: %w", string(out), err)
}
out, err = exec.Command("lvcreate", "--wipesignatures", "y", "-n", "thinpoolmeta", "storage", "--extents", fmt.Sprintf("%d%%VG", cfg.ThinpMetaPercent)).CombinedOutput()
if err != nil {
return errors.Wrap(err, string(out))
return fmt.Errorf("%v: %w", string(out), err)
}

out, err = exec.Command("lvconvert", "-y", "--zero", "n", "-c", "512K", "--thinpool", "storage/thinpool", "--poolmetadata", "storage/thinpoolmeta").CombinedOutput()
if err != nil {
return errors.Wrap(err, string(out))
return fmt.Errorf("%v: %w", string(out), err)
}

profile := fmt.Sprintf("activation{\nthin_pool_autoextend_threshold=%d\nthin_pool_autoextend_percent=%d\n}", cfg.AutoExtendThreshold, cfg.AutoExtendPercent)
err = ioutil.WriteFile(lvmProfileDir+"/storage-thinpool.profile", []byte(profile), 0600)
if err != nil {
return errors.Wrap(err, "error writing storage thinp autoextend profile")
return fmt.Errorf("error writing storage thinp autoextend profile: %w", err)
}

out, err = exec.Command("lvchange", "--metadataprofile", "storage-thinpool", "storage/thinpool").CombinedOutput()
return errors.Wrap(err, string(out))
return fmt.Errorf("%v: %w", string(out), err)

This comment has been minimized.

Copy link
@giuseppe

giuseppe Jul 13, 2022

Member

this is not equivalent I think. errors.Wrap will return nil if err == nil

This comment has been minimized.

Copy link
@saschagrunert

saschagrunert Jul 13, 2022

Author Member

Correct, we should add a nil check before here. Good catch!

This comment has been minimized.

Copy link
@saschagrunert

saschagrunert Jul 13, 2022

Author Member

Closing the loop for the fix: #1286

}

0 comments on commit 3455d12

Please sign in to comment.