feat: error out on qcow2 block devices #300

vyasgun · 2025-04-25T10:54:25Z

Error out when qcow2 image is detected in block device validation
Fixes: Error out when a qcow2 image is detected #290

Summary by Sourcery

Add validation to reject qcow2 formatted block device images.

Bug Fixes:

Prevent vfkit from proceeding with unsupported qcow2 block device images at startup.
Fixes Error out when a qcow2 image is detected #290

Tests:

Add tests for the qcow2 block device validation.

sourcery-ai · 2025-04-25T10:54:29Z

Reviewer's Guide by Sourcery

This 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

Change	Details	Files
Implement validation to detect qcow2 images used as block devices.	Add a new function `ValidBlkDevices` to iterate through VM devices. Check if a VirtioBlk device's image path points to a qcow2 file by reading the header. Return an error if the file header matches the qcow2 magic number.	`pkg/config/config.go`
Integrate the block device validation into the VM configuration process.	Call `ValidBlkDevices` when creating the VM configuration. Return an error if the validation fails.	`cmd/vfkit/main.go`
Add a test case for the new block device validation.	Create a test function `TestVirtualMachine_ValidBlkDevices`. Use a temporary file with qcow2 header to test the error condition. Use a temporary file with a non-qcow header to test the success condition.	`pkg/config/config_test.go`

Possibly linked issues

Error out when a qcow2 image is detected #290: The PR adds validation to error out when qcow2 block devices are detected, directly addressing the issue.

Tips and commands

Interacting with Sourcery

Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. You can also reply to a
review comment with @sourcery-ai issue to create an issue from it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time. You can also comment
@sourcery-ai title on the pull request to (re-)generate the title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time exactly where you
want it. You can also comment @sourcery-ai summary on the pull request to
(re-)generate the summary at any time.
Generate reviewer's guide: Comment @sourcery-ai guide on the pull
request to (re-)generate the reviewer's guide at any time.
Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
pull request to resolve all Sourcery comments. Useful if you've already
addressed all the comments and don't want to see them anymore.
Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
request to dismiss all existing Sourcery reviews. Especially useful if you
want to start fresh with a new review - don't forget to comment
@sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others.
Change the review language.
Add, remove or edit custom review instructions.
Adjust other review settings.

Getting Help

Contact our support team for questions or feedback.
Visit our documentation for detailed guides and information.
Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

openshift-ci · 2025-04-25T10:54:31Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign praveenkumar for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

sourcery-ai

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 VirtioBlk can be combined with the switch statement (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

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

sourcery-ai · 2025-04-25T10:55:02Z

pkg/config/config.go

@@ -250,6 +250,32 @@ func (vm *VirtualMachine) AddIgnitionFileFromCmdLine(cmdlineOpts string) error {
 	return nil
 }

+func (vm *VirtualMachine) ValidBlkDevices() error {


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.

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.

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!

cmd/vfkit/main.go

pkg/config/config_test.go

pkg/config/config.go

nirs · 2025-04-25T14:25:00Z

pkg/config/config.go

@@ -250,6 +250,32 @@ func (vm *VirtualMachine) AddIgnitionFileFromCmdLine(cmdlineOpts string) error {
 	return nil
 }

+func (vm *VirtualMachine) ValidBlkDevices() error {


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

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_test.go

pkg/config/virtio.go

cfergeau · 2025-04-28T15:30:17Z

pkg/config/config.go

@@ -250,6 +250,15 @@ func (vm *VirtualMachine) AddIgnitionFileFromCmdLine(cmdlineOpts string) error {
 	return nil
 }

+func (vm *VirtualMachine) ValidateDevices() error {
+	for _, device := range vm.Devices {
+		if err := device.Validate(); err != nil {


There are a number of calls to Validate in virtio.go, are they made redundant by this new method? or do we need both?

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.

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.

Ah yes, that makes sense. I will add validate back to FromOptions

nirs

Looks much nicer like this!

nirs · 2025-04-28T15:50:13Z

pkg/config/virtio.go

@@ -371,7 +378,7 @@ func (dev *VirtioGPU) FromOptions(options []option) error {
 		dev.Height = defaultVirtioGPUResolutionHeight
 	}

-	return dev.validate()
+	return dev.Validate()


This looks like the right way to validate - after creating from options. Maybe we don't need a public Validate() but add the missing validation in VirtioBlk?

I wrote a public method because it is more conventional and can be used to validate devices from other packages if needed. I can make it private if it's more suitable for this usecase.

pkg/config/config.go

cfergeau · 2025-05-06T15:01:47Z

pkg/config/config.go

@@ -250,6 +250,15 @@ func (vm *VirtualMachine) AddIgnitionFileFromCmdLine(cmdlineOpts string) error {
 	return nil
 }

+func (vm *VirtualMachine) ValidateDevices() error {
+	for _, device := range vm.Devices {
+		if err := device.Validate(); err != nil {


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.

This method allows each VirtioDevice implementation to perform its own validation logic before being attached to the VM. It helps catch misconfigurations early and ensures that invalid devices don't cause runtime errors during VM startup.

Adds a Validate() implementation to the VirtioBlk device that inspects the disk image header. If the image is in qcow2 format, validation fails with an appropriate error.

This function runs validation on all devices added via CmdLine, ensuring they are properly configured before VM startup. It eliminates redundant individual calls to dev.Validate(), centralizing validation logic. Also includes a test case to verify ValidateDevices behavior.

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 · 2025-05-15T08:02:34Z

/retest

vyasgun · 2025-05-15T08:16:40Z

Adding Validate() to FromOptions of VirtioBlkDev is causing the test to fail as the validation checks for the test file /foo/bar to be existent. Will update the test.

openshift-ci bot requested review from lstocchi and praveenkumar April 25, 2025 10:54

sourcery-ai bot reviewed Apr 25, 2025

View reviewed changes

nirs reviewed Apr 25, 2025

View reviewed changes

vyasgun force-pushed the pr/qcow2-error branch from c21d242 to b6e74b8 Compare April 28, 2025 07:16

openshift-merge-robot added the needs-rebase label Apr 28, 2025

cfergeau reviewed Apr 28, 2025

View reviewed changes

nirs reviewed Apr 28, 2025

View reviewed changes

vyasgun force-pushed the pr/qcow2-error branch from b6e74b8 to 7cc757e Compare April 30, 2025 06:27

openshift-merge-robot removed the needs-rebase label Apr 30, 2025

vyasgun force-pushed the pr/qcow2-error branch 6 times, most recently from a914f7f to 38b68ea Compare April 30, 2025 07:45

cfergeau added this to Project planning: crc Apr 30, 2025

cfergeau moved this to Ready for review in Project planning: crc Apr 30, 2025

cfergeau reviewed May 6, 2025

View reviewed changes

vyasgun force-pushed the pr/qcow2-error branch 2 times, most recently from 1f49554 to 49c7338 Compare May 15, 2025 07:54

vyasgun added 5 commits May 15, 2025 13:28

Add Validate() to VirtioDevice interface

aeeb618

This method allows each VirtioDevice implementation to perform its own validation logic before being attached to the VM. It helps catch misconfigurations early and ensures that invalid devices don't cause runtime errors during VM startup.

Error out when qcow2 image is used in a VirtioBlk block device

a9b9c53

Adds a Validate() implementation to the VirtioBlk device that inspects the disk image header. If the image is in qcow2 format, validation fails with an appropriate error.

CI: Install qemu-utils for block device validation tests

21379f2

Adds qemu-utils (for qemu-img) to the CI environment to support new tests that validate block devices and reject unsupported qemu image formats.

Add dev.Validate() back to FromOptions

04c0723

vyasgun force-pushed the pr/qcow2-error branch from 49c7338 to 04c0723 Compare May 15, 2025 07:58

Add test file handling in VirtioBlk device tests

ff9f097

vyasgun force-pushed the pr/qcow2-error branch from 5f16bbf to ff9f097 Compare May 19, 2025 07:46

feat: error out on qcow2 block devices #300

Are you sure you want to change the base?

feat: error out on qcow2 block devices #300

Conversation

vyasgun commented Apr 25, 2025 • edited by sourcery-ai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by Sourcery

Uh oh!

sourcery-ai bot commented Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer's Guide by Sourcery

File-Level Changes

Possibly linked issues

Interacting with Sourcery

Customizing Your Experience

Getting Help

Uh oh!

openshift-ci bot commented Apr 25, 2025

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

sourcery-ai bot Apr 25, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sourcery-ai bot Apr 25, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cfergeau left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nirs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vyasgun commented May 15, 2025

Uh oh!

vyasgun commented May 15, 2025

Uh oh!

Uh oh!

vyasgun commented Apr 25, 2025 •

edited by sourcery-ai bot

Loading

sourcery-ai bot commented Apr 25, 2025 •

edited

Loading