[libvirt] [PATCH] qemu: don't refuse to undefine a guest with NVRAM file

[Cole Robinson]

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