[libvirt] [PATCH] qemu: don't refuse to undefine a guest with NVRAM file
The undefine operation should always be allowed to succeed regardless of whether any NVRAM file exists. ie we should not force the application to use the VIR_DOMAIN_UNDEFINE_NVRAM flag. It is valid for the app to decide it wants the NVRAM file left on disk, in the same way that disk images are left on disk at undefine. --- src/qemu/qemu_driver.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index bec05d4..302bf48 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6985,19 +6985,13 @@ qemuDomainUndefineFlags(virDomainPtr dom, if (!virDomainObjIsActive(vm) && vm->def->os.loader && vm->def->os.loader->nvram && - virFileExists(vm->def->os.loader->nvram)) { - if (!(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot delete inactive domain with nvram")); - goto cleanup; - } - - if (unlink(vm->def->os.loader->nvram) < 0) { - virReportSystemError(errno, - _("failed to remove nvram: %s"), - vm->def->os.loader->nvram); - goto cleanup; - } + virFileExists(vm->def->os.loader->nvram) && + (flags & VIR_DOMAIN_UNDEFINE_NVRAM) && + (unlink(vm->def->os.loader->nvram) < 0)) { + virReportSystemError(errno, + _("failed to remove nvram: %s"), + vm->def->os.loader->nvram); + goto cleanup; } if (virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm) < 0) -- 2.1.0
On Tue, Feb 24, 2015 at 10:12:20 +0000, Daniel Berrange wrote:
The undefine operation should always be allowed to succeed regardless of whether any NVRAM file exists. ie we should not force the application to use the VIR_DOMAIN_UNDEFINE_NVRAM flag. It is valid for the app to decide it wants the NVRAM file left on disk, in the same way that disk images are left on disk at undefine. --- src/qemu/qemu_driver.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index bec05d4..302bf48 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6985,19 +6985,13 @@ qemuDomainUndefineFlags(virDomainPtr dom,
if (!virDomainObjIsActive(vm) && vm->def->os.loader && vm->def->os.loader->nvram && - virFileExists(vm->def->os.loader->nvram)) { - if (!(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot delete inactive domain with nvram")); - goto cleanup; - } - - if (unlink(vm->def->os.loader->nvram) < 0) { - virReportSystemError(errno, - _("failed to remove nvram: %s"), - vm->def->os.loader->nvram); - goto cleanup; - } + virFileExists(vm->def->os.loader->nvram) && + (flags & VIR_DOMAIN_UNDEFINE_NVRAM) && + (unlink(vm->def->os.loader->nvram) < 0)) { + virReportSystemError(errno, + _("failed to remove nvram: %s"), + vm->def->os.loader->nvram); + goto cleanup; }
if (virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm) < 0)
We have prior art in denying to undefine a domain that has information stored in libvirt-internal locations such as the managed save image and snapshot metadata. While it makes sense to allow removing the VM without deleting the NVRAM file when the user specified an external path, we should avoid doing so if the NVRAM is in the libvirt managed path. (Same with externaly managed snapshots or save images). Peter
On Tue, Feb 24, 2015 at 04:07:02PM +0100, Peter Krempa wrote:
On Tue, Feb 24, 2015 at 10:12:20 +0000, Daniel Berrange wrote:
The undefine operation should always be allowed to succeed regardless of whether any NVRAM file exists. ie we should not force the application to use the VIR_DOMAIN_UNDEFINE_NVRAM flag. It is valid for the app to decide it wants the NVRAM file left on disk, in the same way that disk images are left on disk at undefine. --- src/qemu/qemu_driver.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index bec05d4..302bf48 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6985,19 +6985,13 @@ qemuDomainUndefineFlags(virDomainPtr dom,
if (!virDomainObjIsActive(vm) && vm->def->os.loader && vm->def->os.loader->nvram && - virFileExists(vm->def->os.loader->nvram)) { - if (!(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot delete inactive domain with nvram")); - goto cleanup; - } - - if (unlink(vm->def->os.loader->nvram) < 0) { - virReportSystemError(errno, - _("failed to remove nvram: %s"), - vm->def->os.loader->nvram); - goto cleanup; - } + virFileExists(vm->def->os.loader->nvram) && + (flags & VIR_DOMAIN_UNDEFINE_NVRAM) && + (unlink(vm->def->os.loader->nvram) < 0)) { + virReportSystemError(errno, + _("failed to remove nvram: %s"), + vm->def->os.loader->nvram); + goto cleanup; }
if (virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm) < 0)
We have prior art in denying to undefine a domain that has information stored in libvirt-internal locations such as the managed save image and snapshot metadata.
While it makes sense to allow removing the VM without deleting the NVRAM file when the user specified an external path, we should avoid doing so if the NVRAM is in the libvirt managed path. (Same with externaly managed snapshots or save images).
I'm not a real fan of refusal in the managed save case either. The issue is that we're forcing a policy onto the application and not giving it any chance to define a different policy. ie currently apps have a choice of getting an error, or always deleting the nvram, not giving them any chance to delete guest without deleting nvram which is a valid choice they should be permitted to have. By removing this check we allow apps to make their own policy decision with their more complete view of the world. I don't think the quesiton of managed vs unmanaged storage should even come into it either, because that presupposes the application will only ever use managed storage. OpenStack does not currently use libvirt storage pools at all but does wish to use NVRAM. We should not deny the option to undefine the guest in this case. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, Feb 24, 2015 at 15:11:28 +0000, Daniel Berrange wrote:
On Tue, Feb 24, 2015 at 04:07:02PM +0100, Peter Krempa wrote:
On Tue, Feb 24, 2015 at 10:12:20 +0000, Daniel Berrange wrote:
The undefine operation should always be allowed to succeed regardless of whether any NVRAM file exists. ie we should not force the application to use the VIR_DOMAIN_UNDEFINE_NVRAM flag. It is valid for the app to decide it wants the NVRAM file left on disk, in the same way that disk images are left on disk at undefine. --- src/qemu/qemu_driver.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index bec05d4..302bf48 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6985,19 +6985,13 @@ qemuDomainUndefineFlags(virDomainPtr dom,
if (!virDomainObjIsActive(vm) && vm->def->os.loader && vm->def->os.loader->nvram && - virFileExists(vm->def->os.loader->nvram)) { - if (!(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot delete inactive domain with nvram")); - goto cleanup; - } - - if (unlink(vm->def->os.loader->nvram) < 0) { - virReportSystemError(errno, - _("failed to remove nvram: %s"), - vm->def->os.loader->nvram); - goto cleanup; - } + virFileExists(vm->def->os.loader->nvram) && + (flags & VIR_DOMAIN_UNDEFINE_NVRAM) && + (unlink(vm->def->os.loader->nvram) < 0)) { + virReportSystemError(errno, + _("failed to remove nvram: %s"), + vm->def->os.loader->nvram); + goto cleanup; }
if (virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm) < 0)
We have prior art in denying to undefine a domain that has information stored in libvirt-internal locations such as the managed save image and snapshot metadata.
While it makes sense to allow removing the VM without deleting the NVRAM file when the user specified an external path, we should avoid doing so if the NVRAM is in the libvirt managed path. (Same with externaly managed snapshots or save images).
I'm not a real fan of refusal in the managed save case either. The issue is that we're forcing a policy onto the application and not giving it any chance to define a different policy. ie currently apps have a choice of getting an error, or always deleting the nvram, not giving them any chance to delete guest without deleting nvram which is a valid choice they should be permitted to have. By removing this check we allow apps to make their own policy decision with their more complete view of the world. I don't think the quesiton of managed vs unmanaged storage should even come into it either, because that presupposes the application will only ever use managed storage. OpenStack does not currently use libvirt storage pools at all but does wish to use NVRAM. We should not deny the option to undefine the guest in this case.
In that case we probably should have negated the logic here and delete all the stuff by default and give the user option to leave the data behind. The original motivation is apparently that we should not allow anything that would represent state of the deleted VM to be transferred accidentaly to a new VM with same name. For the save image or snapshots the risk of persisting any data is low as a save image would not function without it's disk and still be somewhat secure as it would contain the whole memory image including security. For the NVRAM though it might uncover data stored there or even make the VM unbootable. I agree that the current state is not ideal as we basically force the user to specify all the necessary flags. I think we can safely avoid displaying the message in cases when it's not stored in the libvirt-internal path but for the internal path I'm not convinced that it would be a great idea to change the default. We can perhaps add a flag that woult either enable all the "UNDEFINE*" flags or perhaps invert the logic of them so the user could leave them behind. Peter
On Tue, Feb 24, 2015 at 04:23:41PM +0100, Peter Krempa wrote:
In that case we probably should have negated the logic here and delete all the stuff by default and give the user option to leave the data behind.
I think that would have been even more unsatisfactory - the semantics of undefine were always just that they just remove the configuration, leaving any other aspect of the VM untouched. Making it delete actual data by default would be a significant semantic change IMHO.
The original motivation is apparently that we should not allow anything that would represent state of the deleted VM to be transferred accidentaly to a new VM with same name. For the save image or snapshots the risk of persisting any data is low as a save image would not function without it's disk and still be somewhat secure as it would contain the whole memory image including security. For the NVRAM though it might uncover data stored there or even make the VM unbootable.
I agree that the current state is not ideal as we basically force the user to specify all the necessary flags. I think we can safely avoid displaying the message in cases when it's not stored in the libvirt-internal path but for the internal path I'm not convinced that it would be a great idea to change the default.
This is the problem with trying to put this kind of policy into libvirt though. It is targetting one use case, but has forgotten other valid use cases. For example, consider if the NVRAM file or the managed save image were stored in a filesystem that was NFS. The application wishes to undefine the config on one host and define it on another host. Any checks of this kind will always be wrong for some portion of use cases.
We can perhaps add a flag that woult either enable all the "UNDEFINE*" flags or perhaps invert the logic of them so the user could leave them behind.
IMHO we should just remove this check, and the one for managed save, from the API entirely and leave such error scenario checking to the higher level applications which understand the context of usage in a way that libvirt itself never can. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, Feb 24, 2015 at 15:31:14 +0000, Daniel Berrange wrote:
On Tue, Feb 24, 2015 at 04:23:41PM +0100, Peter Krempa wrote:
In that case we probably should have negated the logic here and delete all the stuff by default and give the user option to leave the data behind.
I think that would have been even more unsatisfactory - the semantics of undefine were always just that they just remove the configuration, leaving any other aspect of the VM untouched. Making it delete actual data by default would be a significant semantic change IMHO.
My wording wasn't clear enough. I was thinking that the initial design should be like this. Changing this is not an option now.
The original motivation is apparently that we should not allow anything that would represent state of the deleted VM to be transferred accidentaly to a new VM with same name. For the save image or snapshots the risk of persisting any data is low as a save image would not function without it's disk and still be somewhat secure as it would contain the whole memory image including security. For the NVRAM though it might uncover data stored there or even make the VM unbootable.
I agree that the current state is not ideal as we basically force the user to specify all the necessary flags. I think we can safely avoid displaying the message in cases when it's not stored in the libvirt-internal path but for the internal path I'm not convinced that it would be a great idea to change the default.
This is the problem with trying to put this kind of policy into libvirt though. It is targetting one use case, but has forgotten other valid use cases. For example, consider if the NVRAM file or the managed save image were stored in a filesystem that was NFS. The application wishes to undefine the config on one host and define it on another host. Any checks of this kind will always be wrong for some portion of use cases.
The mgmt app has the option to use either non-managed save or store the NVRAM in a non-default location for example ...
We can perhaps add a flag that woult either enable all the "UNDEFINE*" flags or perhaps invert the logic of them so the user could leave them behind.
IMHO we should just remove this check, and the one for managed save, from the API entirely and leave such error scenario checking to the higher level applications which understand the context of usage in a way that libvirt itself never can.
Well, okay, but then you should note in the docs that by doing the undefine without the flags some data in libvirt internal paths may be left behind and persist into a new VM with the same name. Peter
On Tue, Feb 24, 2015 at 04:41:44PM +0100, Peter Krempa wrote:
On Tue, Feb 24, 2015 at 15:31:14 +0000, Daniel Berrange wrote:
On Tue, Feb 24, 2015 at 04:23:41PM +0100, Peter Krempa wrote:
The original motivation is apparently that we should not allow anything that would represent state of the deleted VM to be transferred accidentaly to a new VM with same name. For the save image or snapshots the risk of persisting any data is low as a save image would not function without it's disk and still be somewhat secure as it would contain the whole memory image including security. For the NVRAM though it might uncover data stored there or even make the VM unbootable.
I agree that the current state is not ideal as we basically force the user to specify all the necessary flags. I think we can safely avoid displaying the message in cases when it's not stored in the libvirt-internal path but for the internal path I'm not convinced that it would be a great idea to change the default.
This is the problem with trying to put this kind of policy into libvirt though. It is targetting one use case, but has forgotten other valid use cases. For example, consider if the NVRAM file or the managed save image were stored in a filesystem that was NFS. The application wishes to undefine the config on one host and define it on another host. Any checks of this kind will always be wrong for some portion of use cases.
The mgmt app has the option to use either non-managed save or store the NVRAM in a non-default location for example ...
I remember that in the virDomainCreateFlags we have START_FORCE_BOOT which is defined to discard any existing managed save file. Should we either extend that to also discard the NVRAM file, or alternatively add a START_RESET_NVRAM flag as a way to boot with clean BIOS ? Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, Feb 24, 2015 at 15:54:24 +0000, Daniel Berrange wrote:
On Tue, Feb 24, 2015 at 04:41:44PM +0100, Peter Krempa wrote:
On Tue, Feb 24, 2015 at 15:31:14 +0000, Daniel Berrange wrote:
On Tue, Feb 24, 2015 at 04:23:41PM +0100, Peter Krempa wrote:
The original motivation is apparently that we should not allow anything that would represent state of the deleted VM to be transferred accidentaly to a new VM with same name. For the save image or snapshots the risk of persisting any data is low as a save image would not function without it's disk and still be somewhat secure as it would contain the whole memory image including security. For the NVRAM though it might uncover data stored there or even make the VM unbootable.
I agree that the current state is not ideal as we basically force the user to specify all the necessary flags. I think we can safely avoid displaying the message in cases when it's not stored in the libvirt-internal path but for the internal path I'm not convinced that it would be a great idea to change the default.
This is the problem with trying to put this kind of policy into libvirt though. It is targetting one use case, but has forgotten other valid use cases. For example, consider if the NVRAM file or the managed save image were stored in a filesystem that was NFS. The application wishes to undefine the config on one host and define it on another host. Any checks of this kind will always be wrong for some portion of use cases.
The mgmt app has the option to use either non-managed save or store the NVRAM in a non-default location for example ...
I remember that in the virDomainCreateFlags we have START_FORCE_BOOT which is defined to discard any existing managed save file.
Should we either extend that to also discard the NVRAM file, or alternatively add a START_RESET_NVRAM flag as a way to boot with clean BIOS ?
I'd vote for a new one. You might want to discard the save file and keep nvram settings. The other combination will be invalid at least for qemu as the NVRAM will be contained in the save image. Peter
On 02/24/2015 10:23 AM, Peter Krempa wrote:
On Tue, Feb 24, 2015 at 15:11:28 +0000, Daniel Berrange wrote:
On Tue, Feb 24, 2015 at 04:07:02PM +0100, Peter Krempa wrote:
On Tue, Feb 24, 2015 at 10:12:20 +0000, Daniel Berrange wrote:
The undefine operation should always be allowed to succeed regardless of whether any NVRAM file exists. ie we should not force the application to use the VIR_DOMAIN_UNDEFINE_NVRAM flag. It is valid for the app to decide it wants the NVRAM file left on disk, in the same way that disk images are left on disk at undefine. --- src/qemu/qemu_driver.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index bec05d4..302bf48 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6985,19 +6985,13 @@ qemuDomainUndefineFlags(virDomainPtr dom,
if (!virDomainObjIsActive(vm) && vm->def->os.loader && vm->def->os.loader->nvram && - virFileExists(vm->def->os.loader->nvram)) { - if (!(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot delete inactive domain with nvram")); - goto cleanup; - } - - if (unlink(vm->def->os.loader->nvram) < 0) { - virReportSystemError(errno, - _("failed to remove nvram: %s"), - vm->def->os.loader->nvram); - goto cleanup; - } + virFileExists(vm->def->os.loader->nvram) && + (flags & VIR_DOMAIN_UNDEFINE_NVRAM) && + (unlink(vm->def->os.loader->nvram) < 0)) { + virReportSystemError(errno, + _("failed to remove nvram: %s"), + vm->def->os.loader->nvram); + goto cleanup; }
if (virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm) < 0)
We have prior art in denying to undefine a domain that has information stored in libvirt-internal locations such as the managed save image and snapshot metadata.
While it makes sense to allow removing the VM without deleting the NVRAM file when the user specified an external path, we should avoid doing so if the NVRAM is in the libvirt managed path. (Same with externaly managed snapshots or save images).
I'm not a real fan of refusal in the managed save case either. The issue is that we're forcing a policy onto the application and not giving it any chance to define a different policy. ie currently apps have a choice of getting an error, or always deleting the nvram, not giving them any chance to delete guest without deleting nvram which is a valid choice they should be permitted to have. By removing this check we allow apps to make their own policy decision with their more complete view of the world. I don't think the quesiton of managed vs unmanaged storage should even come into it either, because that presupposes the application will only ever use managed storage. OpenStack does not currently use libvirt storage pools at all but does wish to use NVRAM. We should not deny the option to undefine the guest in this case.
In that case we probably should have negated the logic here and delete all the stuff by default and give the user option to leave the data behind.
The original motivation is apparently that we should not allow anything that would represent state of the deleted VM to be transferred accidentaly to a new VM with same name. For the save image or snapshots the risk of persisting any data is low as a save image would not function without it's disk and still be somewhat secure as it would contain the whole memory image including security. For the NVRAM though it might uncover data stored there or even make the VM unbootable.
I agree that the current state is not ideal as we basically force the user to specify all the necessary flags. I think we can safely avoid displaying the message in cases when it's not stored in the libvirt-internal path but for the internal path I'm not convinced that it would be a great idea to change the default.
We can perhaps add a flag that woult either enable all the "UNDEFINE*" flags or perhaps invert the logic of them so the user could leave them behind.
FWIW a flag for UNDEFINE_REMOVE_ALL_STATE or something would definitely be useful for virt-manager. The managed save, snapshot, and nvram undefine errors all hit us and required patches, it would be useful to have one flag that future proofs us. Thanks, Cole
[/me still reading the complete discussion.] On Tue, Feb 24, 2015 at 04:07:02PM +0100, Peter Krempa wrote: [. . .]
We have prior art in denying to undefine a domain that has information stored in libvirt-internal locations such as the managed save image and snapshot metadata.
While it makes sense to allow removing the VM without deleting the NVRAM file when the user specified an external path, we should avoid doing so if the NVRAM is in the libvirt managed path. (Same with externaly managed snapshots or save images).
I see, that explains, as I noticed it[*] when I placed the 'nvram' file in /var/lib/libvirt/ $ virsh dumpxml node2 | grep -i nvram -A2 -B3 <os> <type arch='aarch64' machine='virt'>hvm</type> <loader readonly='yes' type='pflash'>/usr/share/AAVMF/AAVMF_CODE.fd</loader> <nvram>/var/lib/libvirt/nvram/fedora-21-aarch64-nvram</nvram> <boot dev='hd'/> </os> [*] https://bugzilla.redhat.com/show_bug.cgi?id=1195667 -- libvirt fails to undefine a guest with `nvram` XML element -- /kashyap
On Tue, Feb 24, 2015 at 10:12:20AM +0000, Daniel P. Berrange wrote:
The undefine operation should always be allowed to succeed regardless of whether any NVRAM file exists. ie we should not force the application to use the VIR_DOMAIN_UNDEFINE_NVRAM flag. It is valid for the app to decide it wants the NVRAM file left on disk, in the same way that disk images are left on disk at undefine. --- src/qemu/qemu_driver.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index bec05d4..302bf48 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6985,19 +6985,13 @@ qemuDomainUndefineFlags(virDomainPtr dom,
if (!virDomainObjIsActive(vm) && vm->def->os.loader && vm->def->os.loader->nvram && - virFileExists(vm->def->os.loader->nvram)) { - if (!(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot delete inactive domain with nvram")); - goto cleanup; - } - - if (unlink(vm->def->os.loader->nvram) < 0) { - virReportSystemError(errno, - _("failed to remove nvram: %s"), - vm->def->os.loader->nvram); - goto cleanup; - } + virFileExists(vm->def->os.loader->nvram) && + (flags & VIR_DOMAIN_UNDEFINE_NVRAM) && + (unlink(vm->def->os.loader->nvram) < 0)) { + virReportSystemError(errno, + _("failed to remove nvram: %s"), + vm->def->os.loader->nvram); + goto cleanup; }
if (virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm) < 0)
With the above patch applied, I built libvirt RPMs, installed and restarted libvirtd then I tried the below test (which failed) an AArch64 Fedora21 guest. Edit the Fedora 21 libvirt guest. Guest XML here[*]: $ virsh edit devstack [. . .] Try to add the below fragment under 'os' element: <loader readonly='yes' type='pflash'>/usr/share/AAVMF/AAVMF_CODE.fd</loader> <nvram>/var/lib/libvirt/nvram/fedora-21-aarch64-nvram</nvram> It fails to validate the XML: error: XML document failed to validate against schema: Unable to validate doc against /usr/share/libvirt/schemas/domain.rng Extra element devices in interleave Element domain failed to validate content Failed. Try again? [y,n,i,f,?]: However, when I downgrade the libvirt RPMs to version 1.2.11-1.fc22.aarch64 and attempted to add the same XML fragment as above, it works just fine. Tested with: $ uname -r; rpm -q libvirt-daemon-kvm qemu-system-aarch64 3.18.6-200.fc21.aarch64 libvirt-daemon-kvm-1.2.13-1.fc21.aarch64 qemu-system-aarch64-2.2.0-5.fc22.aarch64 libvirt git after I applied your patch: $ git describe v1.2.13-rc1-1-gb63296f [*] The XML I'm tring to edit: ----------------------------------------------------------- <domain type='kvm'> <name>devstack</name> <uuid>d02624c5-460e-436b-baaa-973cfee554c8</uuid> <memory unit='KiB'>8388608</memory> <currentMemory unit='KiB'>8388608</currentMemory> <vcpu placement='static'>1</vcpu> <os> <type arch='aarch64' machine='virt'>hvm</type> <boot dev='hd'/> </os> <features> <acpi/> <apic/> <pae/> </features> <cpu mode='custom' match='exact'> <model fallback='allow'>host</model> </cpu> <clock offset='utc'/> <on_poweroff>destroy</on_poweroff> <on_reboot>restart</on_reboot> <on_crash>restart</on_crash> <devices> <emulator>/usr/bin/qemu-system-aarch64</emulator> <disk type='file' device='disk'> <driver name='qemu' type='qcow2' cache='writeback'/> <source file='/home/kashyapc/devstack'/> <target dev='vda' bus='virtio'/> <address type='virtio-mmio'/> </disk> <interface type='network'> <mac address='52:54:00:f5:6f:23'/> <source network='default'/> <model type='virtio'/> <address type='virtio-mmio'/> </interface> <serial type='pty'> <target port='0'/> </serial> <console type='pty'> <target type='serial' port='0'/> </console> </devices> </domain> ----------------------------------------------------------- -- /kashyap
On Tue, Feb 24, 2015 at 17:46:12 +0100, Kashyap Chamarthy wrote:
On Tue, Feb 24, 2015 at 10:12:20AM +0000, Daniel P. Berrange wrote:
The undefine operation should always be allowed to succeed regardless of whether any NVRAM file exists. ie we should not force the application to use the VIR_DOMAIN_UNDEFINE_NVRAM flag. It is valid for the app to decide it wants the NVRAM file left on disk, in the same way that disk images are left on disk at undefine. --- src/qemu/qemu_driver.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index bec05d4..302bf48 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6985,19 +6985,13 @@ qemuDomainUndefineFlags(virDomainPtr dom,
if (!virDomainObjIsActive(vm) && vm->def->os.loader && vm->def->os.loader->nvram && - virFileExists(vm->def->os.loader->nvram)) { - if (!(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot delete inactive domain with nvram")); - goto cleanup; - } - - if (unlink(vm->def->os.loader->nvram) < 0) { - virReportSystemError(errno, - _("failed to remove nvram: %s"), - vm->def->os.loader->nvram); - goto cleanup; - } + virFileExists(vm->def->os.loader->nvram) && + (flags & VIR_DOMAIN_UNDEFINE_NVRAM) && + (unlink(vm->def->os.loader->nvram) < 0)) { + virReportSystemError(errno, + _("failed to remove nvram: %s"), + vm->def->os.loader->nvram); + goto cleanup; }
if (virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm) < 0)
With the above patch applied, I built libvirt RPMs, installed and restarted libvirtd then I tried the below test (which failed) an AArch64 Fedora21 guest.
Edit the Fedora 21 libvirt guest. Guest XML here[*]:
$ virsh edit devstack [. . .]
Try to add the below fragment under 'os' element:
<loader readonly='yes' type='pflash'>/usr/share/AAVMF/AAVMF_CODE.fd</loader> <nvram>/var/lib/libvirt/nvram/fedora-21-aarch64-nvram</nvram>
It fails to validate the XML:
error: XML document failed to validate against schema: Unable to validate doc against /usr/share/libvirt/schemas/domain.rng Extra element devices in interleave Element domain failed to validate content
Failed. Try again? [y,n,i,f,?]:
The <loader> and <nvram> need to be put after the <os><type ... > element due to the ordering in libvirt's schema. I'll look into the schema whether we can make the interleave span the <type> element too. Until then either put it in the right place or use the 'i' key to ignore the schema check. Peter
On Wed, Feb 25, 2015 at 11:11:37AM +0100, Peter Krempa wrote:
On Tue, Feb 24, 2015 at 17:46:12 +0100, Kashyap Chamarthy wrote:
On Tue, Feb 24, 2015 at 10:12:20AM +0000, Daniel P. Berrange wrote:
The undefine operation should always be allowed to succeed regardless of whether any NVRAM file exists. ie we should not force the application to use the VIR_DOMAIN_UNDEFINE_NVRAM flag. It is valid for the app to decide it wants the NVRAM file left on disk, in the same way that disk images are left on disk at undefine. --- src/qemu/qemu_driver.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index bec05d4..302bf48 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6985,19 +6985,13 @@ qemuDomainUndefineFlags(virDomainPtr dom,
if (!virDomainObjIsActive(vm) && vm->def->os.loader && vm->def->os.loader->nvram && - virFileExists(vm->def->os.loader->nvram)) { - if (!(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot delete inactive domain with nvram")); - goto cleanup; - } - - if (unlink(vm->def->os.loader->nvram) < 0) { - virReportSystemError(errno, - _("failed to remove nvram: %s"), - vm->def->os.loader->nvram); - goto cleanup; - } + virFileExists(vm->def->os.loader->nvram) && + (flags & VIR_DOMAIN_UNDEFINE_NVRAM) && + (unlink(vm->def->os.loader->nvram) < 0)) { + virReportSystemError(errno, + _("failed to remove nvram: %s"), + vm->def->os.loader->nvram); + goto cleanup; }
if (virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm) < 0)
With the above patch applied, I built libvirt RPMs, installed and restarted libvirtd then I tried the below test (which failed) an AArch64 Fedora21 guest.
Edit the Fedora 21 libvirt guest. Guest XML here[*]:
$ virsh edit devstack [. . .]
Try to add the below fragment under 'os' element:
<loader readonly='yes' type='pflash'>/usr/share/AAVMF/AAVMF_CODE.fd</loader> <nvram>/var/lib/libvirt/nvram/fedora-21-aarch64-nvram</nvram>
It fails to validate the XML:
error: XML document failed to validate against schema: Unable to validate doc against /usr/share/libvirt/schemas/domain.rng Extra element devices in interleave Element domain failed to validate content
Failed. Try again? [y,n,i,f,?]:
The <loader> and <nvram> need to be put after the <os><type ... > element due to the ordering in libvirt's schema.
Actually I tested that too, and forgot to mention -- validation still fails the same way, desipte the above ordering. This patch from Martin Kletzander fixes it: https://www.redhat.com/archives/libvir-list/2015-February/msg01022.html
I'll look into the schema whether we can make the interleave span the <type> element too.
Until then either put it in the right place or use the 'i' key to ignore the schema check.
Yes, 'i' is what I did to test it. Thanks. -- /kashyap
On Tue, Feb 24, 2015 at 05:46:12PM +0100, Kashyap Chamarthy wrote:
On Tue, Feb 24, 2015 at 10:12:20AM +0000, Daniel P. Berrange wrote:
[. . .]
Edit the Fedora 21 libvirt guest. Guest XML here[*]:
$ virsh edit devstack [. . .]
Try to add the below fragment under 'os' element:
<loader readonly='yes' type='pflash'>/usr/share/AAVMF/AAVMF_CODE.fd</loader> <nvram>/var/lib/libvirt/nvram/fedora-21-aarch64-nvram</nvram>
It fails to validate the XML:
error: XML document failed to validate against schema: Unable to validate doc against /usr/share/libvirt/schemas/domain.rng Extra element devices in interleave Element domain failed to validate content
Failed. Try again? [y,n,i,f,?]:
I ignored the validation here 'i' (thanks Michal Privoznik for the suggestion on IRC -- this looks like something to do with some XML validation work from Martin Kletzander), and went ahead to test the actual bug this patch fixes: It works: A guest w/ 'nvmram' XML element as below: $ virsh dumpxml node2 | grep -i nvram -A2 -B3 <os> <type arch='aarch64' machine='virt'>hvm</type> <loader readonly='yes' type='pflash'>/usr/share/AAVMF/AAVMF_CODE.fd</loader> <nvram>/var/lib/libvirt/nvram/fedora-21-aarch64-nvram</nvram> <boot dev='hd'/> </os> Can be undefined successfully. -- /kashyap
24.02.2015 13:12, Daniel P. Berrange пишет:
The undefine operation should always be allowed to succeed regardless of whether any NVRAM file exists. ie we should not force the application to use the VIR_DOMAIN_UNDEFINE_NVRAM flag. It is valid for the app to decide it wants the NVRAM file left on disk, in the same way that disk images are left on disk at undefine. --- src/qemu/qemu_driver.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index bec05d4..302bf48 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6985,19 +6985,13 @@ qemuDomainUndefineFlags(virDomainPtr dom,
if (!virDomainObjIsActive(vm) && vm->def->os.loader && vm->def->os.loader->nvram && - virFileExists(vm->def->os.loader->nvram)) { - if (!(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot delete inactive domain with nvram")); - goto cleanup; - } - - if (unlink(vm->def->os.loader->nvram) < 0) { - virReportSystemError(errno, - _("failed to remove nvram: %s"), - vm->def->os.loader->nvram); - goto cleanup; - } + virFileExists(vm->def->os.loader->nvram) && + (flags & VIR_DOMAIN_UNDEFINE_NVRAM) && + (unlink(vm->def->os.loader->nvram) < 0)) { + virReportSystemError(errno, + _("failed to remove nvram: %s"), + vm->def->os.loader->nvram); + goto cleanup; }
if (virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm) < 0)
As I found out the discussion followed this patch didn't come to a conclusion and this or any other patches on the matter weren't commited. We hit the problem with inability to undefine a domain leaving nvram untouched recently and this patch would solve it perfectly. I think it's worth commiting IMHO and maybe the documentation should reflect this slight change in behavior. Any new thoughts? Maxim
On 23.05.2016 11:16, Maxim Nestratov wrote:
24.02.2015 13:12, Daniel P. Berrange пишет:
The undefine operation should always be allowed to succeed regardless of whether any NVRAM file exists. ie we should not force the application to use the VIR_DOMAIN_UNDEFINE_NVRAM flag. It is valid for the app to decide it wants the NVRAM file left on disk, in the same way that disk images are left on disk at undefine. --- src/qemu/qemu_driver.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index bec05d4..302bf48 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6985,19 +6985,13 @@ qemuDomainUndefineFlags(virDomainPtr dom, if (!virDomainObjIsActive(vm) && vm->def->os.loader && vm->def->os.loader->nvram && - virFileExists(vm->def->os.loader->nvram)) { - if (!(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot delete inactive domain with nvram")); - goto cleanup; - } - - if (unlink(vm->def->os.loader->nvram) < 0) { - virReportSystemError(errno, - _("failed to remove nvram: %s"), - vm->def->os.loader->nvram); - goto cleanup; - } + virFileExists(vm->def->os.loader->nvram) && + (flags & VIR_DOMAIN_UNDEFINE_NVRAM) && + (unlink(vm->def->os.loader->nvram) < 0)) { + virReportSystemError(errno, + _("failed to remove nvram: %s"), + vm->def->os.loader->nvram); + goto cleanup; } if (virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm) < 0)
As I found out the discussion followed this patch didn't come to a conclusion and this or any other patches on the matter weren't commited. We hit the problem with inability to undefine a domain leaving nvram untouched recently and this patch would solve it perfectly. I think it's worth commiting IMHO and maybe the documentation should reflect this slight change in behavior. Any new thoughts?
Does this Cole's suggestion sounds reasonable? https://www.redhat.com/archives/libvir-list/2015-February/msg00974.html Michal
23.05.2016 19:37, Michal Privoznik пишет:
On 23.05.2016 11:16, Maxim Nestratov wrote:
24.02.2015 13:12, Daniel P. Berrange пишет:
The undefine operation should always be allowed to succeed regardless of whether any NVRAM file exists. ie we should not force the application to use the VIR_DOMAIN_UNDEFINE_NVRAM flag. It is valid for the app to decide it wants the NVRAM file left on disk, in the same way that disk images are left on disk at undefine. --- src/qemu/qemu_driver.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index bec05d4..302bf48 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6985,19 +6985,13 @@ qemuDomainUndefineFlags(virDomainPtr dom, if (!virDomainObjIsActive(vm) && vm->def->os.loader && vm->def->os.loader->nvram && - virFileExists(vm->def->os.loader->nvram)) { - if (!(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot delete inactive domain with nvram")); - goto cleanup; - } - - if (unlink(vm->def->os.loader->nvram) < 0) { - virReportSystemError(errno, - _("failed to remove nvram: %s"), - vm->def->os.loader->nvram); - goto cleanup; - } + virFileExists(vm->def->os.loader->nvram) && + (flags & VIR_DOMAIN_UNDEFINE_NVRAM) && + (unlink(vm->def->os.loader->nvram) < 0)) { + virReportSystemError(errno, + _("failed to remove nvram: %s"), + vm->def->os.loader->nvram); + goto cleanup; } if (virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm) < 0) As I found out the discussion followed this patch didn't come to a conclusion and this or any other patches on the matter weren't commited. We hit the problem with inability to undefine a domain leaving nvram untouched recently and this patch would solve it perfectly. I think it's worth commiting IMHO and maybe the documentation should reflect this slight change in behavior. Any new thoughts? Does this Cole's suggestion sounds reasonable?
https://www.redhat.com/archives/libvir-list/2015-February/msg00974.html
Michal
As far as I understand the proposed flag would eliminate the necessity to specify all flags (SNAPSHOT, SAVE_STATE, NVRAM and some future ones) to force undefining. But this won't allow us to undefine a domain and left a nvram file untouched. Maxim
On 05/23/2016 12:37 PM, Michal Privoznik wrote:
On 23.05.2016 11:16, Maxim Nestratov wrote:
24.02.2015 13:12, Daniel P. Berrange пишет:
The undefine operation should always be allowed to succeed regardless of whether any NVRAM file exists. ie we should not force the application to use the VIR_DOMAIN_UNDEFINE_NVRAM flag. It is valid for the app to decide it wants the NVRAM file left on disk, in the same way that disk images are left on disk at undefine. --- src/qemu/qemu_driver.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index bec05d4..302bf48 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6985,19 +6985,13 @@ qemuDomainUndefineFlags(virDomainPtr dom, if (!virDomainObjIsActive(vm) && vm->def->os.loader && vm->def->os.loader->nvram && - virFileExists(vm->def->os.loader->nvram)) { - if (!(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot delete inactive domain with nvram")); - goto cleanup; - } - - if (unlink(vm->def->os.loader->nvram) < 0) { - virReportSystemError(errno, - _("failed to remove nvram: %s"), - vm->def->os.loader->nvram); - goto cleanup; - } + virFileExists(vm->def->os.loader->nvram) && + (flags & VIR_DOMAIN_UNDEFINE_NVRAM) && + (unlink(vm->def->os.loader->nvram) < 0)) { + virReportSystemError(errno, + _("failed to remove nvram: %s"), + vm->def->os.loader->nvram); + goto cleanup; } if (virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm) < 0)
As I found out the discussion followed this patch didn't come to a conclusion and this or any other patches on the matter weren't commited. We hit the problem with inability to undefine a domain leaving nvram untouched recently and this patch would solve it perfectly. I think it's worth commiting IMHO and maybe the documentation should reflect this slight change in behavior. Any new thoughts?
Does this Cole's suggestion sounds reasonable?
https://www.redhat.com/archives/libvir-list/2015-February/msg00974.html
That UNDEFINE_REMOVE_STATE or whatever flag would definitely be useful, but it's kind of separate. Dan's points elsewhere in the thread are valid, that the API doesn't provide any way to undefine a VM but _not_ remove autocreated nvram file. That seems potentially problematic. However before Dan's patch could be applied, someone needs to verify how libvirt handles the case when redefining a new VM with UEFI but there's already an old nvram vars file in place... does libvirt error, or silently use the old one, or silently overwrite the old one? It may need extra patches, or more documentation - Cole
participants (6)
-
Cole Robinson -
Daniel P. Berrange -
Kashyap Chamarthy -
Maxim Nestratov -
Michal Privoznik -
Peter Krempa