From 4ddaf4d41f1b578835266ab37caba69643ac2e54 Mon Sep 17 00:00:00 2001 From: Ruslan Bayandinov Date: Fri, 27 Jan 2023 10:09:59 +0000 Subject: [PATCH 1/3] refactor errors with stack Signed-off-by: Ruslan Bayandinov --- pkg/networkservice/common/mechanisms/vfio/server.go | 4 +++- pkg/sriov/pci/pool.go | 6 ++++-- pkg/tools/cgroup/cgroup.go | 12 ++++++++---- pkg/tools/cgroup/fake_cgroup.go | 6 ++++-- pkg/tools/cgroup/file_api.go | 9 ++++++--- 5 files changed, 25 insertions(+), 12 deletions(-) diff --git a/pkg/networkservice/common/mechanisms/vfio/server.go b/pkg/networkservice/common/mechanisms/vfio/server.go index e9bd9540..fabc3326 100644 --- a/pkg/networkservice/common/mechanisms/vfio/server.go +++ b/pkg/networkservice/common/mechanisms/vfio/server.go @@ -1,5 +1,7 @@ // Copyright (c) 2020-2022 Doc.ai and/or its affiliates. // +// Copyright (c) 2023 Cisco and/or its affiliates. +// // SPDX-License-Identifier: Apache-2.0 // // Licensed under the Apache License, Version 2.0 (the "License"); @@ -114,7 +116,7 @@ func (s *vfioServer) Request(ctx context.Context, request *networkservice.Networ func (s *vfioServer) getDeviceNumbers(deviceFile string) (major, minor uint32, err error) { info := new(unix.Stat_t) if err := unix.Stat(deviceFile, info); err != nil { - return 0, 0, err + return 0, 0, errors.WithStack(err) } return Major(info.Rdev), Minor(info.Rdev), nil } diff --git a/pkg/sriov/pci/pool.go b/pkg/sriov/pci/pool.go index da3c61f0..252bb9c4 100644 --- a/pkg/sriov/pci/pool.go +++ b/pkg/sriov/pci/pool.go @@ -2,6 +2,8 @@ // // Copyright (c) 2021 Nordix Foundation. // +// Copyright (c) 2023 Cisco and/or its affiliates. +// // SPDX-License-Identifier: Apache-2.0 // // Licensed under the Apache License, Version 2.0 (the "License"); @@ -190,7 +192,7 @@ func (p *Pool) waitDriverGettingBound(ctx context.Context, pcif pciFunction, dri select { case <-ctx.Done(): - return ctx.Err() + return errors.WithStack(ctx.Err()) case <-timeoutCh: return errors.Errorf("time for binding kernel driver exceeded: %s, cause: %v", pcif.GetPCIAddress(), err) case <-time.After(driverBindCheck): @@ -218,5 +220,5 @@ func (p *Pool) vfioDriverCheck(pcif pciFunction) error { } _, err = os.Stat(filepath.Join(p.vfioDir, strconv.FormatUint(uint64(iommuGroup), 10))) - return err + return errors.WithStack(err) } diff --git a/pkg/tools/cgroup/cgroup.go b/pkg/tools/cgroup/cgroup.go index 6428c4ea..3e67cd4c 100644 --- a/pkg/tools/cgroup/cgroup.go +++ b/pkg/tools/cgroup/cgroup.go @@ -1,5 +1,7 @@ // Copyright (c) 2020-2022 Doc.ai and/or its affiliates. // +// Copyright (c) 2023 Cisco and/or its affiliates. +// // SPDX-License-Identifier: Apache-2.0 // // Licensed under the Apache License, Version 2.0 (the "License"); @@ -25,6 +27,8 @@ import ( "os" "path/filepath" "reflect" + + "github.com/pkg/errors" ) const ( @@ -43,7 +47,7 @@ func NewCgroups(pathPattern string) (cgroups []*Cgroup, err error) { var filePaths []string filePaths, err = filepath.Glob(filepath.Join(pathPattern, deviceListFileName)) if err != nil { - return nil, err + return nil, errors.WithStack(err) } for _, filePath := range filePaths { @@ -57,14 +61,14 @@ func NewCgroups(pathPattern string) (cgroups []*Cgroup, err error) { func (c *Cgroup) Allow(major, minor uint32) error { dev := newDevice(major, minor, 'r', 'w', 'm') - return ioutil.WriteFile(filepath.Join(c.Path, deviceAllowFileName), []byte(dev.String()), 0) + return errors.WithStack(ioutil.WriteFile(filepath.Join(c.Path, deviceAllowFileName), []byte(dev.String()), 0)) } // Deny denies "c major:minor rw" for cgroup func (c *Cgroup) Deny(major, minor uint32) error { dev := newDevice(major, minor, 'r', 'w') - return ioutil.WriteFile(filepath.Join(c.Path, deviceDenyFileName), []byte(dev.String()), 0) + return errors.WithStack(ioutil.WriteFile(filepath.Join(c.Path, deviceDenyFileName), []byte(dev.String()), 0)) } // IsAllowed returns if "c major:minor rwm" is allowed for cgroup @@ -88,7 +92,7 @@ func (c *Cgroup) IsWiderThan(major, minor uint32) (bool, error) { func (c *Cgroup) compareTo(dev *device) (isAllowed, isWider bool, err error) { file, err := os.Open(filepath.Join(c.Path, deviceListFileName)) if err != nil { - return false, false, err + return false, false, errors.WithStack(err) } defer func() { _ = file.Close() }() diff --git a/pkg/tools/cgroup/fake_cgroup.go b/pkg/tools/cgroup/fake_cgroup.go index 8debbc0b..8eb1ca55 100644 --- a/pkg/tools/cgroup/fake_cgroup.go +++ b/pkg/tools/cgroup/fake_cgroup.go @@ -1,5 +1,7 @@ // Copyright (c) 2020-2022 Doc.ai and/or its affiliates. // +// Copyright (c) 2023 Cisco and/or its affiliates. +// // SPDX-License-Identifier: Apache-2.0 // // Licensed under the Apache License, Version 2.0 (the "License"); @@ -62,7 +64,7 @@ func NewFakeWideCgroup(ctx context.Context, path string) (*Cgroup, error) { } if err = deviceSupplier("a *:* rwm"); err != nil { - return nil, err + return nil, errors.WithStack(err) } return cg, nil @@ -70,7 +72,7 @@ func NewFakeWideCgroup(ctx context.Context, path string) (*Cgroup, error) { func newFakeCgroup(ctx context.Context, path string) (*Cgroup, supplierFunc, error) { if err := os.MkdirAll(path, mkdirPerm); err != nil { - return nil, nil, err + return nil, nil, errors.WithStack(err) } go func() { <-ctx.Done() diff --git a/pkg/tools/cgroup/file_api.go b/pkg/tools/cgroup/file_api.go index 24a81f1f..5e07559a 100644 --- a/pkg/tools/cgroup/file_api.go +++ b/pkg/tools/cgroup/file_api.go @@ -1,5 +1,7 @@ // Copyright (c) 2020-2022 Doc.ai and/or its affiliates. // +// Copyright (c) 2023 Cisco and/or its affiliates. +// // SPDX-License-Identifier: Apache-2.0 // // Licensed under the Apache License, Version 2.0 (the "License"); @@ -25,6 +27,7 @@ import ( "io/ioutil" "os" + "github.com/pkg/errors" "golang.org/x/sys/unix" ) @@ -36,12 +39,12 @@ func inputFileAPI(ctx context.Context, filePath string, consumer func(string)) e _ = os.Remove(filePath) err := unix.Mkfifo(filePath, createPerm) if err != nil { - return err + return errors.WithStack(err) } fd, err := unix.Open(filePath, unix.O_RDWR|unix.O_NONBLOCK, 0) if err != nil { - return err + return errors.WithStack(err) } file := os.NewFile(uintptr(fd), filePath) @@ -79,7 +82,7 @@ type supplierFunc func(s string) error func outputFileAPI(filePath string) (supplier supplierFunc) { supplier = func(data string) error { - return ioutil.WriteFile(filePath, []byte(data), createPerm) + return errors.WithStack(ioutil.WriteFile(filePath, []byte(data), createPerm)) } return supplier } From 805314cd9bbdc3bb59e51c04d6a15429108548e0 Mon Sep 17 00:00:00 2001 From: Ruslan Bayandinov Date: Fri, 27 Jan 2023 10:10:38 +0000 Subject: [PATCH 2/3] update generated files Signed-off-by: Ruslan Bayandinov --- pkg/networkservice/common/resetmechanism/mechanism_map.gen.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/networkservice/common/resetmechanism/mechanism_map.gen.go b/pkg/networkservice/common/resetmechanism/mechanism_map.gen.go index fbf9b066..a18df77e 100644 --- a/pkg/networkservice/common/resetmechanism/mechanism_map.gen.go +++ b/pkg/networkservice/common/resetmechanism/mechanism_map.gen.go @@ -1,4 +1,6 @@ // Code generated by "-output mechanism_map.gen.go -type mechanismMap -output mechanism_map.gen.go -type mechanismMap"; DO NOT EDIT. +// Install -output mechanism_map.gen.go -type mechanismMap by "go get -u github.com/searKing/golang/tools/-output mechanism_map.gen.go -type mechanismMap" + package resetmechanism import ( From 506b2ab4a22e6cf85f2defe06af71e87ba2c5bd3 Mon Sep 17 00:00:00 2001 From: Ruslan Bayandinov Date: Thu, 2 Feb 2023 07:03:43 +0000 Subject: [PATCH 3/3] update errors handling Signed-off-by: Ruslan Bayandinov --- .../common/mechanisms/vfio/client.go | 9 ++++--- .../common/mechanisms/vfio/server.go | 2 +- pkg/sriov/pci/pool.go | 4 ++-- pkg/sriov/pcifunction/function.go | 6 +++-- pkg/sriov/pcifunction/physical_function.go | 4 +++- pkg/tools/cgroup/cgroup.go | 24 ++++++++++++++----- pkg/tools/cgroup/fake_cgroup.go | 4 ++-- pkg/tools/cgroup/file_api.go | 9 ++++--- 8 files changed, 42 insertions(+), 20 deletions(-) diff --git a/pkg/networkservice/common/mechanisms/vfio/client.go b/pkg/networkservice/common/mechanisms/vfio/client.go index a435a9e7..77120aaf 100644 --- a/pkg/networkservice/common/mechanisms/vfio/client.go +++ b/pkg/networkservice/common/mechanisms/vfio/client.go @@ -1,5 +1,7 @@ // Copyright (c) 2020-2022 Doc.ai and/or its affiliates. // +// Copyright (c) 2023 Cisco and/or its affiliates. +// // SPDX-License-Identifier: Apache-2.0 // // Licensed under the Apache License, Version 2.0 (the "License"); @@ -26,6 +28,7 @@ import ( "github.com/golang/protobuf/ptypes/empty" "github.com/networkservicemesh/sdk/pkg/networkservice/utils/inject/injecterror" + "github.com/pkg/errors" "golang.org/x/sys/unix" "google.golang.org/grpc" @@ -89,7 +92,7 @@ func (c *vfioClient) Request(ctx context.Context, request *networkservice.Networ if mech := vfio.ToMechanism(conn.GetMechanism()); mech != nil { if err := os.Mkdir(c.vfioDir, mkdirPerm); err != nil && !os.IsExist(err) { logger.Error("failed to create vfio directory") - return nil, err + return nil, errors.Wrapf(err, "failed to create vfio directory %s", c.vfioDir) } if err := unix.Mknod( @@ -98,7 +101,7 @@ func (c *vfioClient) Request(ctx context.Context, request *networkservice.Networ int(unix.Mkdev(mech.GetVfioMajor(), mech.GetVfioMinor())), ); err != nil && !os.IsExist(err) { logger.Errorf("failed to mknod device: %v", vfioDevice) - return nil, err + return nil, errors.Wrapf(err, "failed to mknod device: %v", vfioDevice) } igid := mech.GetParameters()[vfio.IommuGroupKey] @@ -108,7 +111,7 @@ func (c *vfioClient) Request(ctx context.Context, request *networkservice.Networ int(unix.Mkdev(mech.GetDeviceMajor(), mech.GetDeviceMinor())), ); err != nil && !os.IsExist(err) { logger.Errorf("failed to mknod device: %v", vfioDevice) - return nil, err + return nil, errors.Wrapf(err, "failed to mknod device: %v", vfioDevice) } } diff --git a/pkg/networkservice/common/mechanisms/vfio/server.go b/pkg/networkservice/common/mechanisms/vfio/server.go index fabc3326..a8e71a58 100644 --- a/pkg/networkservice/common/mechanisms/vfio/server.go +++ b/pkg/networkservice/common/mechanisms/vfio/server.go @@ -116,7 +116,7 @@ func (s *vfioServer) Request(ctx context.Context, request *networkservice.Networ func (s *vfioServer) getDeviceNumbers(deviceFile string) (major, minor uint32, err error) { info := new(unix.Stat_t) if err := unix.Stat(deviceFile, info); err != nil { - return 0, 0, errors.WithStack(err) + return 0, 0, errors.Wrapf(err, "failed to check %s file status", deviceFile) } return Major(info.Rdev), Minor(info.Rdev), nil } diff --git a/pkg/sriov/pci/pool.go b/pkg/sriov/pci/pool.go index 252bb9c4..77eb9b6a 100644 --- a/pkg/sriov/pci/pool.go +++ b/pkg/sriov/pci/pool.go @@ -192,7 +192,7 @@ func (p *Pool) waitDriverGettingBound(ctx context.Context, pcif pciFunction, dri select { case <-ctx.Done(): - return errors.WithStack(ctx.Err()) + return errors.Wrap(ctx.Err(), "provided context is done") case <-timeoutCh: return errors.Errorf("time for binding kernel driver exceeded: %s, cause: %v", pcif.GetPCIAddress(), err) case <-time.After(driverBindCheck): @@ -220,5 +220,5 @@ func (p *Pool) vfioDriverCheck(pcif pciFunction) error { } _, err = os.Stat(filepath.Join(p.vfioDir, strconv.FormatUint(uint64(iommuGroup), 10))) - return errors.WithStack(err) + return errors.Wrapf(err, "failed to join path elements: %s, %s", p.vfioDir, strconv.FormatUint(uint64(iommuGroup), 10)) } diff --git a/pkg/sriov/pcifunction/function.go b/pkg/sriov/pcifunction/function.go index 609628dd..61222063 100644 --- a/pkg/sriov/pcifunction/function.go +++ b/pkg/sriov/pcifunction/function.go @@ -1,5 +1,7 @@ // Copyright (c) 2020-2022 Doc.ai and/or its affiliates. // +// Copyright (c) 2023 Cisco and/or its affiliates. +// // SPDX-License-Identifier: Apache-2.0 // // Licensed under the Apache License, Version 2.0 (the "License"); @@ -71,7 +73,7 @@ func (f *Function) GetNetInterfaceName() (string, error) { func (f *Function) GetIOMMUGroup() (uint, error) { stringIOMMUGroup, err := evalSymlinkAndGetBaseName(f.withDevicePath(iommuGroup)) if err != nil { - return 0, errors.Wrapf(err, "error evaluating IOMMU group id for the device: %v", f.address) + return 0, err } iommuGroup, _ := strconv.Atoi(stringIOMMUGroup) @@ -87,7 +89,7 @@ func (f *Function) GetBoundDriver() (string, error) { driver, err := evalSymlinkAndGetBaseName(f.withDevicePath(boundDriverPath)) if err != nil { - return "", errors.Wrapf(err, "error evaluating bound driver for the device: %v", f.address) + return "", err } return driver, nil diff --git a/pkg/sriov/pcifunction/physical_function.go b/pkg/sriov/pcifunction/physical_function.go index f2140e07..abb31c5a 100644 --- a/pkg/sriov/pcifunction/physical_function.go +++ b/pkg/sriov/pcifunction/physical_function.go @@ -1,5 +1,7 @@ // Copyright (c) 2020-2022 Doc.ai and/or its affiliates. // +// Copyright (c) 2023 Cisco and/or its affiliates. +// // SPDX-License-Identifier: Apache-2.0 // // Licensed under the Apache License, Version 2.0 (the "License"); @@ -97,7 +99,7 @@ func (pf *PhysicalFunction) GetVirtualFunctions() []*Function { func (pf *PhysicalFunction) createVirtualFunctions() error { switch vfsCount, err := readUintFromFile(pf.withDevicePath(configuredVFFile)); { case err != nil: - return errors.Wrapf(err, "failed to get configured VFs number for the PCI device: %v", pf.address) + return err case vfsCount > 0: return nil } diff --git a/pkg/tools/cgroup/cgroup.go b/pkg/tools/cgroup/cgroup.go index 3e67cd4c..cf93618e 100644 --- a/pkg/tools/cgroup/cgroup.go +++ b/pkg/tools/cgroup/cgroup.go @@ -45,9 +45,10 @@ type Cgroup struct { // NewCgroups returns all cgroups matching pathPattern func NewCgroups(pathPattern string) (cgroups []*Cgroup, err error) { var filePaths []string - filePaths, err = filepath.Glob(filepath.Join(pathPattern, deviceListFileName)) + pattern := filepath.Join(pathPattern, deviceListFileName) + filePaths, err = filepath.Glob(pattern) if err != nil { - return nil, errors.WithStack(err) + return nil, errors.Wrapf(err, "failed to get filepaths %s", pattern) } for _, filePath := range filePaths { @@ -61,14 +62,24 @@ func NewCgroups(pathPattern string) (cgroups []*Cgroup, err error) { func (c *Cgroup) Allow(major, minor uint32) error { dev := newDevice(major, minor, 'r', 'w', 'm') - return errors.WithStack(ioutil.WriteFile(filepath.Join(c.Path, deviceAllowFileName), []byte(dev.String()), 0)) + filePath := filepath.Join(c.Path, deviceAllowFileName) + if err := ioutil.WriteFile(filePath, []byte(dev.String()), 0); err != nil { + return errors.Wrapf(err, "failed to write to a %s", filePath) + } + + return nil } // Deny denies "c major:minor rw" for cgroup func (c *Cgroup) Deny(major, minor uint32) error { dev := newDevice(major, minor, 'r', 'w') - return errors.WithStack(ioutil.WriteFile(filepath.Join(c.Path, deviceDenyFileName), []byte(dev.String()), 0)) + filePath := filepath.Join(c.Path, deviceAllowFileName) + if err := ioutil.WriteFile(filePath, []byte(dev.String()), 0); err != nil { + return errors.Wrapf(err, "failed to write to a %s", filePath) + } + + return nil } // IsAllowed returns if "c major:minor rwm" is allowed for cgroup @@ -90,9 +101,10 @@ func (c *Cgroup) IsWiderThan(major, minor uint32) (bool, error) { } func (c *Cgroup) compareTo(dev *device) (isAllowed, isWider bool, err error) { - file, err := os.Open(filepath.Join(c.Path, deviceListFileName)) + filePath := filepath.Clean(filepath.Join(c.Path, deviceListFileName)) + file, err := os.Open(filePath) if err != nil { - return false, false, errors.WithStack(err) + return false, false, errors.Wrapf(err, "failed to open file %s", filePath) } defer func() { _ = file.Close() }() diff --git a/pkg/tools/cgroup/fake_cgroup.go b/pkg/tools/cgroup/fake_cgroup.go index 8eb1ca55..ee266b65 100644 --- a/pkg/tools/cgroup/fake_cgroup.go +++ b/pkg/tools/cgroup/fake_cgroup.go @@ -64,7 +64,7 @@ func NewFakeWideCgroup(ctx context.Context, path string) (*Cgroup, error) { } if err = deviceSupplier("a *:* rwm"); err != nil { - return nil, errors.WithStack(err) + return nil, err } return cg, nil @@ -72,7 +72,7 @@ func NewFakeWideCgroup(ctx context.Context, path string) (*Cgroup, error) { func newFakeCgroup(ctx context.Context, path string) (*Cgroup, supplierFunc, error) { if err := os.MkdirAll(path, mkdirPerm); err != nil { - return nil, nil, errors.WithStack(err) + return nil, nil, errors.Wrapf(err, "failed to create directory path %s", path) } go func() { <-ctx.Done() diff --git a/pkg/tools/cgroup/file_api.go b/pkg/tools/cgroup/file_api.go index 5e07559a..405a548a 100644 --- a/pkg/tools/cgroup/file_api.go +++ b/pkg/tools/cgroup/file_api.go @@ -39,12 +39,12 @@ func inputFileAPI(ctx context.Context, filePath string, consumer func(string)) e _ = os.Remove(filePath) err := unix.Mkfifo(filePath, createPerm) if err != nil { - return errors.WithStack(err) + return errors.Wrapf(err, "failed to make FIFO special file %s", filePath) } fd, err := unix.Open(filePath, unix.O_RDWR|unix.O_NONBLOCK, 0) if err != nil { - return errors.WithStack(err) + return errors.Wrapf(err, "failed to open file %s", filePath) } file := os.NewFile(uintptr(fd), filePath) @@ -82,7 +82,10 @@ type supplierFunc func(s string) error func outputFileAPI(filePath string) (supplier supplierFunc) { supplier = func(data string) error { - return errors.WithStack(ioutil.WriteFile(filePath, []byte(data), createPerm)) + if err := ioutil.WriteFile(filePath, []byte(data), createPerm); err != nil { + return errors.Wrapf(err, "failed to write to a %s", filePath) + } + return nil } return supplier }