Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add fallback ErrorString function #99

Merged
merged 1 commit into from
Feb 12, 2024

Conversation

elezar
Copy link
Member

@elezar elezar commented Feb 12, 2024

This change adds a fallback ErrorString function that is used when the nvmlErrorString function cannot be resolved. For example, when the library is not loaded.

Closes #82

@elezar elezar self-assigned this Feb 12, 2024
@klueska
Copy link
Contributor

klueska commented Feb 12, 2024

Does this mimic what we did in go-nvlib?

@elezar
Copy link
Member Author

elezar commented Feb 12, 2024

Does this mimic what we did in go-nvlib?

The fallbackErrorString function is the same. The mechanics is different since I don't set a global function var but do a symbol lookup with every call. I thought that this was simpler since it didn't require state (the global) function pointer to be updated.

Note that when we originally implemented the logic in go-nvlib, we didn't have the Lookup function exposed at a package level here.

@klueska
Copy link
Contributor

klueska commented Feb 12, 2024

Seems reasonable. Given that we only end up calling this on error conditions, the extra bit of time to look up the symbol should be negligible.

@klueska klueska self-requested a review February 12, 2024 09:55
klueska
klueska previously approved these changes Feb 12, 2024
pkg/nvml/return.go Outdated Show resolved Hide resolved
gen/nvml/return.go Outdated Show resolved Hide resolved
This change adds a fallback ErrorString function that is used when
the nvmlErrorString function cannot be resolved. For example, when
the library is not loaded.

Signed-off-by: Evan Lezar <[email protected]>
@elezar elezar merged commit aa1e291 into NVIDIA:main Feb 12, 2024
2 checks passed
@yxxhero
Copy link
Contributor

yxxhero commented Feb 20, 2024

@elezar @klueska

// nvml.ErrorString()
func ErrorString(r Return) string {
	if err := GetLibrary().Lookup("nvmlErrorString"); err == nil {
		return nvmlErrorString(r)
	}
	return fallbackErrorStringFunc(r)
}

I think that it should use err == nil.

@elezar elezar deleted the fallback-error-string branch February 20, 2024 07:56
@elezar
Copy link
Member Author

elezar commented Feb 20, 2024

Yes, you're right. Thanks for your PR. I think it would be better to invert the return statements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nvmlErrorString error
3 participants