refactor: replace string concatenation in monitor CLI option construc… - #858
refactor: replace string concatenation in monitor CLI option construc…#858singhshresth26 wants to merge 1 commit into
Conversation
✅ Deploy Preview for urunc canceled.
|
c87d026 to
158fe2f
Compare
|
Hi @cmainas, just following up on this PR, would love your feedback whenever you’re available. |
cmainas
left a comment
cmainas
left a comment
There was a problem hiding this comment.
Hello @singhshresth26 ,
thank you for the PR. I have added some comments and a generic request. Please do not use Sprintf directly in append, instead construct the string first and then add append it. The code becomes much cleaner.
Also, can you rebase over main and apply the changes to hyperlight and take into consideration the changes in Solo5?
| } | ||
| extraMonArgs := ukernel.MonitorCli() | ||
| exArgs = append(exArgs, extraMonArgs.OtherArgs...) | ||
| exArgs = append(exArgs, args.UnikernelPath) |
There was a problem hiding this comment.
Let;s add the -- which is the Solo5 separator for Solo5 cli options and unikernel cl options.
|
|
||
| if args.VCPUs > 0 { | ||
| cmdString += fmt.Sprintf(" -smp %d", args.VCPUs) | ||
| exArgs = append(exArgs, "-smp", fmt.Sprintf("%d", args.VCPUs)) |
There was a problem hiding this comment.
Use strconv instead of fmt for integer to string convention.
| blockCli1 := fmt.Sprintf(" -device virtio-blk-pci,serial=%s,drive=%s,scsi=off", blockArg.ID, blockArg.ID) | ||
| blockCli2 := fmt.Sprintf(" -drive format=raw,if=none,id=%s,file=%s", blockArg.ID, blockArg.Path) | ||
| blockCli = blockCli1 + blockCli2 | ||
| if len(blockCli) == 0 && blockArg.ID != "" && blockArg.Path != "" { |
There was a problem hiding this comment.
We should always give priority to ID and Path. and then fallback to blcokCli.
| devType = "virtio-net-device" | ||
| } | ||
| netcli += fmt.Sprintf(" %s,host_mtu=%d,mac=%s", getVirtioNetArg(), args.Net.MTU, args.Net.MAC) | ||
| exArgs = append(exArgs, "-device", fmt.Sprintf("%s,netdev=net0,host_mtu=%d,mac=%s", devType, args.Net.MTU, args.Net.MAC)) |
There was a problem hiding this comment.
Construct the netcli before appending it to make it cleaner.
There was a problem hiding this comment.
Also getVirtioNetArg has been removed.
| cmdArgs := strings.Split(cmdString, " ") | ||
| return cmdArgs, nil | ||
| exArgs = append(exArgs, extraMonArgs.OtherArgs...) | ||
| exArgs = append(exArgs, args.UnikernelPath) |
There was a problem hiding this comment.
Let;s add the -- separator for Solo5 cli options and unikernel cl options.
…tion (urunc-dev#844) Signed-off-by: Shresth Singh <shresthengineer@gmail.com>
158fe2f to
fbbaa3c
Compare
|
Sorry for the delay, I have addressed all your comments. |
2 participants
Description
This PR refactors the monitor/hypervisor command line arguments construction to use string slices directly instead of inefficient string concatenation.
Previously, monitors (such as HVT, SPT, QEMU, Firecracker, and Cloud Hypervisor) built their CLI options as a single space-separated string using
cli += " option"and split it into a string slice at the end usingstrings.Split. This is inefficient because strings are immutable in Go, leading to unnecessary memory allocations, and the split operation adds overhead.This PR:
Unikernelinterface methodMonitorNetClito return[]stringinstead ofstring.MonitorBlockArgs.ExactArgsandMonitorCliArgs.OtherArgsto use[]stringinstead ofstring.Related issues
How was this tested?
Compiled the packages and verified that unit tests build successfully targeting Linux:
go build ./pkg/unikontainers/...- Passed successfully.go test -c ./pkg/unikontainers/...- Passed successfully.Checklist
make lint).make test_ctr,make test_nerdctl,make test_docker,make test_crictl).