-
Notifications
You must be signed in to change notification settings - Fork 174
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
vhost-vdpa: fix DeviceInfo file content #493
Conversation
/retest |
Return error instead and handle it at the call sites. |
@bn222 AFAIK there is no error condition to be handled at the call site. Let's say, if a path is returned, it'll be used to fill some structs, otherwise an empty path will be stored instead. The glog.Infof is just for convenience. |
It seems to me that whenever this function returns "", it was due to it being called on something that doesn't have this path, i.e. it should not have been called on it the first place? |
Added error handling for GetPath function |
4f4bf71
to
f26c266
Compare
/lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
pkg/infoprovider/vdpaInfoProvider.go
Outdated
vdpaPath, err := vip.dev.GetPath() | ||
if err != nil { | ||
glog.Errorf("Unexpected error when fetching the vdpa device path: %s", err) | ||
return devSpecs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we return nil explicitly like above ? (When device not healthy)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be safer, will do the change thanks!
The path attribute in the DeviceInfo specification is now set to something like '/dev/vhost-vdpa-0'. See issue k8snetworkplumbingwg#492 Signed-off-by: Leonardo Milleri <lmilleri@redhat.com>
Pull Request Test Coverage Report for Build 5588346567
💛 - Coveralls |
This PR fixes an issue in the DeviceInfo file generation for vhost-vdpa devices.
For details please check the following issue:
#492