feat: error out on qcow2 block devices #300
Conversation
Reviewer's Guide by SourceryThis pull request adds validation to detect qcow2 block devices used with VirtioBlk and returns an error if found, as vfkit does not support this format. It also includes tests for the new validation logic. No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @vyasgun - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider defining the qcow2 magic number
"QFI\xfb"as a constant for better readability and maintainability. - The type assertion for
VirtioBlkcan be combined with theswitchstatement (e.g.,case dev := device.(*VirtioBlk):) for improved conciseness.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
pkg/config/config.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func (vm *VirtualMachine) ValidBlkDevices() error { |
There was a problem hiding this comment.
issue (complexity): Consider using a type switch with an assignment and moving the file validation logic into its own helper function to improve code structure.
Try using a type switch with an assignment and move the file‐validation logic into its own helper. For example:
func (vm *VirtualMachine) ValidBlkDevices() error {
for _, device := range vm.Devices {
switch dev := device.(type) {
case *VirtioBlk:
if dev.ImagePath == "" {
continue
}
if err := validateBlkDevice(dev.ImagePath); err != nil {
return err
}
}
}
return nil
}
func validateBlkDevice(imgPath string) error {
file, err := os.Open(imgPath)
if err != nil {
return fmt.Errorf("failed to open file %s: %v", imgPath, err)
}
defer file.Close()
header := make([]byte, 4)
if _, err := file.Read(header); err != nil {
return fmt.Errorf("failed to read the header of file %s: %v", imgPath, err)
}
if string(header) == "QFI\xfb" {
return fmt.Errorf("vfkit does not support qcow2 block devices")
}
return nil
}This reduces redundant assertions and flattens the nested ifs, while keeping functionality intact.
There was a problem hiding this comment.
This is a good advice. Even better is to add a Validate() interface to devices so we can keep device related code in the same place.
There was a problem hiding this comment.
That's a great point, @nirs! Adding a Validate() method to a device interface would definitely be a cleaner and more scalable solution in the long run. It keeps the validation logic tightly coupled with the device implementation itself.
My suggestion aimed for a more localized improvement within the existing structure, but your interface-based approach is certainly preferable for future maintainability and extensibility. If you have the bandwidth to implement that, it would be an excellent enhancement!
pkg/config/config.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func (vm *VirtualMachine) ValidBlkDevices() error { |
There was a problem hiding this comment.
This is a good advice. Even better is to add a Validate() interface to devices so we can keep device related code in the same place.
cfergeau
left a comment
cfergeau
left a comment
There was a problem hiding this comment.
Can you split this PR in at least 2 commits, one adding the Validate method to the VirtioDevice interface (+ValidateDevices), and a second commit adding the qcow2 check? I’d even move ValidateDevices to its own commit so that each commit does a single thing.
pkg/config/config.go
Outdated
|
|
||
| func (vm *VirtualMachine) ValidateDevices() error { | ||
| for _, device := range vm.Devices { | ||
| if err := device.Validate(); err != nil { |
There was a problem hiding this comment.
There are a number of calls to Validate in virtio.go, are they made redundant by this new method? or do we need both?
There was a problem hiding this comment.
I think either the Validate() calls at the end of FromOptions in some places can be removed or we can add dev.Validate to the end of all FromOptions. I have removed the redundant occurrences in this iteration.
There was a problem hiding this comment.
One concern I have with this is that this weakens the FromOptions and AddDevicesFromCommandLine APIs, which are kind of public: I expect other go programs to be using github.com/crc-org/pkg/config, even if I’m not sure these specific methods will be widely used.
Before this change, they were returning an error when the config does not validate, after this change, all users of the API need to remember to do the validation themselves or they could get invalid device configuration.
I would keep the validation at the end of each FromOptions, or add the validation to either AddDevicesFromCmdLine or deviceFromCmdLine if we want to avoid the duplication.
There was a problem hiding this comment.
Ah yes, that makes sense. I will add validate back to FromOptions
nirs
left a comment
nirs
left a comment
There was a problem hiding this comment.
Looks much nicer like this!
vyasgun
force-pushed
the
pr/qcow2-error
branch
6 times, most recently
from
a914f7f to
38b68ea
Compare
pkg/config/config.go
Outdated
|
|
||
| func (vm *VirtualMachine) ValidateDevices() error { | ||
| for _, device := range vm.Devices { | ||
| if err := device.Validate(); err != nil { |
There was a problem hiding this comment.
One concern I have with this is that this weakens the FromOptions and AddDevicesFromCommandLine APIs, which are kind of public: I expect other go programs to be using github.com/crc-org/pkg/config, even if I’m not sure these specific methods will be widely used.
Before this change, they were returning an error when the config does not validate, after this change, all users of the API need to remember to do the validation themselves or they could get invalid device configuration.
I would keep the validation at the end of each FromOptions, or add the validation to either AddDevicesFromCmdLine or deviceFromCmdLine if we want to avoid the duplication.
vyasgun
force-pushed
the
pr/qcow2-error
branch
3 times, most recently
from
49c7338 to
04c0723
Compare
|
/retest |
|
Adding |
cfergeau
left a comment
cfergeau
left a comment
There was a problem hiding this comment.
The Add dev.Validate() back to FromOptions undoes what was done in Add ValidateDevices function to validate devices parsed from CmdLine , could you merge these 2 commits somehow?
Added validation logic to `VirtioBlk` to detect and reject qcow2 image files
based on their magic header ("QFI\xfb").
Adds qemu-utils (for qemu-img) to the CI environment to support new tests that validate block devices and reject unsupported qemu image formats.
vyasgun
force-pushed
the
pr/qcow2-error
branch
3 times, most recently
from
e37bed1 to
f786434
Compare
|
@cfergeau addressed the comments. |
cfergeau
left a comment
cfergeau
left a comment
There was a problem hiding this comment.
Only a few minor comments left
Added unit tests for the new `VirtioBlk.validate` method to ensure qcow2 images are correctly rejected based on their magic header. Updated existing VirtioBlk-related tests to create temporary image files on disk, as the new validation logic checks for file existence and reads the header. This ensures the tests pass with the added file-based validation in place.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cfergeau The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
github-project-automation
bot
moved this from Ready for review
to Done
in Project planning: crc
4 participants
Summary by Sourcery
Add validation to reject qcow2 formatted block device images.
Bug Fixes:
Tests: