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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list