-
Notifications
You must be signed in to change notification settings - Fork 1
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
User input error check for discovery flag #6
base: dev/refector-multi-endpoint
Are you sure you want to change the base?
User input error check for discovery flag #6
Conversation
cmd/qat_plugin/qat_plugin.go
Outdated
@@ -365,7 +371,7 @@ func main() { | |||
kernelVfDrivers := flag.String("kernel-vf-drivers", "dh895xccvf,c6xxvf,c3xxxvf,d15xxvf", "Comma separated VF Device Driver of the QuickAssist Devices in the system. Devices supported: DH895xCC,C62x,C3xxx and D15xx") | |||
maxNumDevices := flag.Int("max-num-devices", 32, "maximum number of QAT devices to be provided to the QuickAssist device plugin") | |||
debugEnabled := flag.Bool("debug", false, "enable debug output") | |||
discovery := flag.String("discovery", "generic", "generic or per-pf or per-device") | |||
discovery := flag.String("discovery", "generic,per-pf,per-device", "generic or per-pf or per-device") |
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.
The default discovery mechanism should be "generic" not all three discovery mechanisms. We don't want to over-complicate this as if we have resources exposed per pf, generic and per device, consumption from a certain pool would impact other pool as well
cmd/qat_plugin/qat_plugin.go
Outdated
@@ -376,6 +382,14 @@ func main() { | |||
fmt.Println("Wrong DPDK device driver:", *dpdkDriver) | |||
os.Exit(1) | |||
} | |||
|
|||
discoveryFlags := strings.Split(*discovery, ",") | |||
for _, discovery := range discoveryFlags { |
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.
This loop would not be needed as we assume that there is only one discovery flag provided by the user, if no flag is provided, the default discovery flag would be generic
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.
Thanks for the PR Conor, just a few minor changes but other than that it looks good to me!
d140187
to
4a53077
Compare
cmd/qat_plugin/qat_plugin.go
Outdated
|
||
if !isValidDiscoveryFlag(*discovery) { | ||
fmt.Printf("Invalid discovery flag: %v Discovery set to default: generic\n",*discovery) | ||
*discovery = "generic" |
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.
Sorry for the miscommunication. We would need to do os.exist here if the user input is incorrect as we need to ensure that the user enters correct values and the plugin is doing exactly what is expected, if the user enters no value it defaults to generic as it happens in line 374
LGTM! Can you please squash the commits and I will merge the PR. |
183d4ac
to
00305cf
Compare
No description provided.