Skip to content

Commit bfe32f4

Browse files
shiv-tyagishiv-tyagi
committed
fix: wrap errors for proper error chain propagation
We should not just print on console and return the error as-is to upstream but rather wrap it with context and then propagate it. This is a standard golang practice.
1 parent c894621 commit bfe32f4

8 files changed

Lines changed: 90 additions & 124 deletions

File tree

‎cmd/amd-ctk/cdi/generate/generate.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ func AddNewCommand() *cli.Command {
6363
func validateGenOptions(c *cli.Context, genOptions *generateOptions) error {
6464
_, err := filepath.Abs(genOptions.output)
6565
if err != nil {
66-
return fmt.Errorf("incorrect output file, Err: %v", err)
66+
return fmt.Errorf("incorrect output file: %w", err)
6767
}
6868

6969
return nil
@@ -72,19 +72,19 @@ func validateGenOptions(c *cli.Context, genOptions *generateOptions) error {
7272
func performAction(c *cli.Context, genOptions *generateOptions) error {
7373
cdi, err := cdi.New(genOptions.output)
7474
if err != nil {
75-
return fmt.Errorf("Failed to create CDI handler, Error: %v", err)
75+
return fmt.Errorf("failed to create CDI handler: %w", err)
7676
}
7777

7878
// Generate CDI spec
7979
err = cdi.GenerateSpec()
8080
if err != nil {
81-
return fmt.Errorf("Failed to generate CDI spec, Error: %v", err)
81+
return fmt.Errorf("failed to generate CDI spec: %w", err)
8282
}
8383

8484
// Write updated CDI spec
8585
err = cdi.WriteSpec()
8686
if err != nil {
87-
return fmt.Errorf("Failed to write generated runtime CDI spec, Error: %v", err)
87+
return fmt.Errorf("failed to write CDI spec: %w", err)
8888
}
8989

9090
return nil

‎cmd/amd-ctk/cdi/validate/validate.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ func AddNewCommand() *cli.Command {
6363
func validateValOptions(c *cli.Context, valOptions *validateOptions) error {
6464
_, err := filepath.Abs(valOptions.cdiSpecPath)
6565
if err != nil {
66-
return fmt.Errorf("Incorrect CDI spec file, Error: %v", err)
66+
return fmt.Errorf("incorrect CDI spec file: %w", err)
6767
}
6868

6969
return nil
@@ -72,13 +72,13 @@ func validateValOptions(c *cli.Context, valOptions *validateOptions) error {
7272
func performAction(c *cli.Context, valOptions *validateOptions) error {
7373
cdi, err := cdi.New(valOptions.cdiSpecPath)
7474
if err != nil {
75-
return fmt.Errorf("Failed to create CDI handler, Error: %v", err)
75+
return fmt.Errorf("failed to create CDI handler: %w", err)
7676
}
7777

7878
// Validate CDI spec
7979
result, err := cdi.ValidateSpec()
8080
if err != nil {
81-
return fmt.Errorf("Failed to validate CDI spec")
81+
return fmt.Errorf("failed to validate CDI spec: %w", err)
8282
}
8383
if result == true {
8484
fmt.Printf("CDI spec is valid\n")

‎internal/amdgpu/amdgpu.go‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ func (fs *DefaultFS) GetDeviceStat(dev string, format string) (string, error) {
6464
out, err := exec.Command("stat", "-c", format, dev).Output()
6565
if err != nil {
6666
logger.Log.Printf("stat failed for %v, Error: %v", dev, err)
67-
return "", err
67+
return "", fmt.Errorf("stat device %s (format %s): %w", dev, format, err)
6868
}
6969
return strings.TrimSpace(string(out)), nil
7070
}
@@ -98,7 +98,7 @@ func GetAMDGPUs() ([]DeviceInfo, error) {
9898
func GetAMDGPUsWithFS(fs FileSystem) ([]DeviceInfo, error) {
9999
if _, err := fs.Stat("/sys/module/amdgpu/drivers/"); err != nil {
100100
logger.Log.Printf("amdgpu driver unavailable: %s", err)
101-
return nil, err
101+
return nil, fmt.Errorf("amdgpu driver not found: %w", err)
102102
}
103103

104104
renderDevIds := GetDevIdsFromTopology(fs)
@@ -111,7 +111,7 @@ func GetAMDGPUsWithFS(fs FileSystem) ([]DeviceInfo, error) {
111111
pciDevs, err := fs.Glob("/sys/module/amdgpu/drivers/pci:amdgpu/[0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F]:*")
112112
if err != nil {
113113
logger.Log.Printf("Failed to find amdgpu driver directories: %s", err)
114-
return nil, err
114+
return nil, fmt.Errorf("read amdgpu driver info: %w", err)
115115
}
116116

117117
// Process platform devices for partitions
@@ -144,7 +144,7 @@ func GetAMDGPUsWithFS(fs FileSystem) ([]DeviceInfo, error) {
144144
drms, err := fs.Glob(path + "/drm/*")
145145
if err != nil {
146146
logger.Log.Printf("Failed to find amdgpu driver drm directories: %s", err)
147-
return nil, err
147+
return nil, fmt.Errorf("read device info from %s: %w", path, err)
148148
}
149149

150150
drmDevs := []string{}
@@ -223,8 +223,8 @@ func GetAMDGPUWithFS(fs FileSystem, dev string) (AMDGPU, error) {
223223
return 0
224224
}
225225

226-
return ret
227-
}
226+
return ret
227+
}
228228

229229
gpu := AMDGPU{
230230
Path: dev,
@@ -291,7 +291,7 @@ func GetDevIdsFromTopology(fs FileSystem, topoRootParam ...string) map[int]strin
291291
func ParseTopologyProperties(fs FileSystem, path string, re *regexp.Regexp) (int64, error) {
292292
content, err := fs.ReadFile(path)
293293
if err != nil {
294-
return 0, err
294+
return 0, fmt.Errorf("read property from %s: %w", path, err)
295295
}
296296

297297
scanner := bufio.NewScanner(strings.NewReader(string(content)))
@@ -310,7 +310,7 @@ func ParseTopologyProperties(fs FileSystem, path string, re *regexp.Regexp) (int
310310
func ParseTopologyPropertiesString(fs FileSystem, path string, re *regexp.Regexp) (string, error) {
311311
content, err := fs.ReadFile(path)
312312
if err != nil {
313-
return "", err
313+
return "", fmt.Errorf("read property from %s: %w", path, err)
314314
}
315315

316316
scanner := bufio.NewScanner(strings.NewReader(string(content)))
@@ -333,7 +333,7 @@ func GetUniqueIdToDeviceIndexMap() (map[string][]int, error) {
333333
func GetUniqueIdToDeviceIndexMapWithFS(fs FileSystem) (map[string][]int, error) {
334334
devs, err := GetAMDGPUsWithFS(fs)
335335
if err != nil {
336-
return nil, err
336+
return nil, fmt.Errorf("lookup device: %w", err)
337337
}
338338

339339
renderDevIds := GetDevIdsFromTopology(fs)

‎internal/amdgpu/amdgpu_test.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -333,7 +333,7 @@ func TestGetAMDGPUs(t *testing.T) {
333333

334334
if tt.expectedError != nil {
335335
assert.Error(t, err)
336-
assert.Equal(t, tt.expectedError, err)
336+
assert.ErrorIs(t, err, tt.expectedError)
337337
} else {
338338
assert.NoError(t, err)
339339
assert.Equal(t, tt.expectedDevs, devs)
@@ -513,7 +513,7 @@ func TestGetAMDGPUWithFS(t *testing.T) {
513513

514514
if tt.expectedError != nil {
515515
assert.Error(t, err)
516-
assert.Equal(t, tt.expectedError, err)
516+
assert.ErrorIs(t, err, tt.expectedError)
517517
} else {
518518
assert.NoError(t, err)
519519
assert.Equal(t, tt.expectedGPU, gpu)
@@ -638,7 +638,7 @@ func TestGetUniqueIdToDeviceIndexMapWithFS(t *testing.T) {
638638

639639
if tt.expectedError != nil {
640640
assert.Error(t, err)
641-
assert.Equal(t, tt.expectedError, err)
641+
assert.ErrorIs(t, err, tt.expectedError)
642642
} else {
643643
assert.NoError(t, err)
644644
assert.Equal(t, tt.expectedResult, result)

‎internal/cdi/cdi.go‎

Lines changed: 11 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -77,14 +77,14 @@ type cdi_t struct {
7777
func readSpecFromFile(f string) (*specs.Spec, error) {
7878
file, err := os.Open(f)
7979
if err != nil {
80-
return &specs.Spec{}, err
80+
return &specs.Spec{}, fmt.Errorf("open CDI spec file %s: %w", f, err)
8181
}
8282
defer file.Close()
8383

8484
var spec specs.Spec
8585
err = json.NewDecoder(file).Decode(&spec)
8686
if err != nil {
87-
return &specs.Spec{}, err
87+
return &specs.Spec{}, fmt.Errorf("decode CDI spec from %s: %w", f, err)
8888
}
8989

9090
return &spec, nil
@@ -94,14 +94,14 @@ func (cdi *cdi_t) GenerateSpec() error {
9494
gpus, err := cdi.getGPUs()
9595
if err != nil {
9696
logger.Log.Printf("Failed to get GPUs, Err: %v", err)
97-
return err
97+
return fmt.Errorf("get GPUs for CDI spec: %w", err)
9898
}
9999

100100
getCDIDevNode := func(gpu string) (specs.DeviceNode, error) {
101101
d, err := cdi.getGPU(gpu)
102102
if err != nil {
103103
logger.Log.Printf("Failed to get details of %v GPU, Err: %v", gpu, err)
104-
return specs.DeviceNode{}, err
104+
return specs.DeviceNode{}, fmt.Errorf("get device node for %s: %w", gpu, err)
105105
}
106106

107107
dn := specs.DeviceNode{
@@ -120,7 +120,7 @@ func (cdi *cdi_t) GenerateSpec() error {
120120

121121
kfdDeviceNode, err := getCDIDevNode("/dev/kfd")
122122
if err != nil {
123-
return err
123+
return fmt.Errorf("get KFD device node: %w", err)
124124
}
125125

126126
cdiDevs := []specs.Device{}
@@ -130,7 +130,7 @@ func (cdi *cdi_t) GenerateSpec() error {
130130
for _, gpu := range gpuList.DrmDevices {
131131
dn, err := getCDIDevNode(gpu)
132132
if err != nil {
133-
return err
133+
return fmt.Errorf("get device node for GPU %s: %w", gpu, err)
134134
}
135135
dnl = append(dnl, &dn)
136136
}
@@ -171,15 +171,14 @@ func (cdi *cdi_t) WriteSpec() error {
171171
file, err := os.Create(cdi.specPath)
172172
if err != nil {
173173
logger.Log.Printf("Error creating file, Error: %v", err)
174-
return err
174+
return fmt.Errorf("create CDI spec file %s: %w", cdi.specPath, err)
175175
}
176176

177177
defer file.Close()
178178

179179
encoder := json.NewEncoder(file)
180180
if err := encoder.Encode(cdi.spec); err != nil {
181-
fmt.Printf("Error encoding JSON: %s\n", err)
182-
return err
181+
return fmt.Errorf("encode CDI spec to %s: %w", cdi.specPath, err)
183182
}
184183

185184
logger.Log.Printf("Wrote spec to %v", cdi.specPath)
@@ -191,7 +190,7 @@ func (cdi *cdi_t) PrintSpec() error {
191190
prettyJSON, err := json.MarshalIndent(cdi.spec, "", " ")
192191
if err != nil {
193192
logger.Log.Printf("Failed to marshal JSON, Error: %v", err)
194-
return err
193+
return fmt.Errorf("marshal CDI spec to JSON: %w", err)
195194
}
196195

197196
fmt.Printf(string(prettyJSON))
@@ -234,8 +233,8 @@ func New(sp string) (Interface, error) {
234233
if _, err := os.Stat(dir); os.IsNotExist(err) {
235234
err := os.MkdirAll(dir, 0755)
236235
if err != nil {
237-
logger.Log.Printf("Failed to create %v, Err: %v", dir, err)
238-
return nil, err
236+
logger.Log.Printf("Failed to create %v, Err: %v", sp, err)
237+
return nil, fmt.Errorf("create CDI spec directory %s: %w", sp, err)
239238
}
240239
}
241240

0 commit comments

Comments
 (0)