-
Notifications
You must be signed in to change notification settings - Fork 42
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
Filter out UNDEFINE_NVRAM for test:///default #153
Conversation
https://gitlab.com/libvirt/libvirt/-/blob/239669049d9904e5e8da2d8b2a38d4d927a167e9/src/qemu/qemu_capabilities.c#L261 suggests there's an nvram capability, but I don't see this on my real libvirt instance. https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainUndefineFlagsValues is a list of possible flags, but this really doesn't help since I don't see a way to probe for supported flags. |
@@ -97,6 +97,11 @@ def destroy(options={ :destroy_volumes => false, :flags => ::Libvirt::Domain::UN | |||
if flags.zero? | |||
service.vm_action(uuid, :undefine) | |||
else | |||
# the test driver doesn't support UNDEFINE_NVRAM and we can't probe |
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.
Should we have a "FIXME" comment so that it gets highlighted?
Is it possible to open a issue for the test driver so that the UNDEFINE_NVRAM gets fixed soonish?
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.
Some more digging in the source tells me it's implemented in src/test. Specifically, https://gitlab.com/libvirt/libvirt/-/blob/239669049d9904e5e8da2d8b2a38d4d927a167e9/src/test/test_driver.c?page=5#L4578-4634
I'm not sure if it can simply be added to virCheckFlags
there, or if the whole driver needs to support nvram.
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 this is true. The https://gitlab.com/libvirt/libvirt/-/blob/master/src/internal.h?ref_type=heads#L259 is a just a macro and will return if a unsupported flag is used. And https://gitlab.com/libvirt/libvirt/-/blob/239669049d9904e5e8da2d8b2a38d4d927a167e9/src/test/test_driver.c?page=5#L4587 only specified some flags but not NVRAM.
@@ -97,6 +97,11 @@ def destroy(options={ :destroy_volumes => false, :flags => ::Libvirt::Domain::UN | |||
if flags.zero? | |||
service.vm_action(uuid, :undefine) | |||
else | |||
# the test driver doesn't support UNDEFINE_NVRAM and we can't probe | |||
# for the capability at runtime | |||
if service.uri.uri == 'test:///default' |
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 did some further digging. https://libvirt.org/drvtest.html documents the test driver. So really it should look at the scheme see if that's test
or test+something
.
The test driver doesn't support UNDEFINE_NVRAM and I couldn't find a way to probe for the capability. This hacks around it by looking at the service uri and filters out the flag. It also adds an explicit test since previously the exception was swallowed. A new server is created to avoid interference with other tests.
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. Thanks for fixing this.
The test driver doesn't support UNDEFINE_NVRAM and I couldn't find a way to probe for the capability. This hacks around it by looking at the service uri and filters out the flag.
It also adds an explicit test since previously the exception was swallowed. A new server is created to avoid interference with other tests.