[libvirt] [PATCH 1/2] qemu: Delete nvram store for transient domains too

[Michal Privoznik]

On 12.11.2014 19:10, Peter Krempa wrote:
> On 11/12/14 18:26, Michal Privoznik wrote:
>> There are two ways how to use nvram variable store file in
>> libvirt:
>>
>>    1) create it by hand and pass the path in domain XML
>>
>>    2) let libvirt generate path and create the file
>>
>> Now, we allow users to remove the file from case 2) by passing a
>> flag into the virDomainUndefineFlags(). But when implementing
>> this I forgot about transient domains. For them the file needs to
>> be removed in qemuProcessStop(). But not always, only if the file
>> was created by us.  For that reason new attribute to <nvram/> is
>> invented: @generated to keep the info between daemon restarts.
>> However, now that we are removing autogenerated file for
>> transient domains automatically, we should do the same for
>> persistent domains too without requiring the
>> VIR_DOMAIN_UNDEFINE_NVRAM flag.
>
> I agree that we should clean up the file once we create it as currently
> the guest would be started with the NVRAM generated for a different
> guest with the same name which might trigger some unwanted behavior.
>
> The existing behavior (introduced in 1.2.8) though might lead the users
> to use transient VMs with unspecified NVRAM path and rely on the fact
> that the NVRAM file is present from the last run.
>
> I think we should document that unless the NVRAM file is provided
> externally the settings don't persist between runs and users using
> transient VMs (hello oVirt) should take care of generating it by
> themselves as they might get unexpected results and we should do it
> rather sooner than later until somebody starts abusing it.
>
> As a side note I find it borderline useful to have such configuration at
> all as it will only survive until the VM is restarted and thus it's kind
> of pointless to write the nvram variables to the disk at all.
>

Not really, transient domain is not destroyed on guest OS reboots. So it 
makes a sense. A little.

>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>   src/conf/domain_conf.c  | 10 ++++++++--
>>   src/conf/domain_conf.h  |  1 +
>>   src/qemu/qemu_driver.c  |  3 ++-
>>   src/qemu/qemu_process.c | 14 +++++++++++---
>>   4 files changed, 22 insertions(+), 6 deletions(-)
>>
>
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index 24e5f0f..c0ab341 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>
>> @@ -3938,7 +3937,7 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
>>                           LOCALSTATEDIR, def->name) < 0)
>>               goto cleanup;
>>
>> -        generated = true;
>> +        loader->generated = true;
>>
>>           if (virDomainSaveConfig(cfg->configDir, def) < 0)
>>               goto cleanup;
>
> Okay, so placement of qemuPrepareNVRAM is critical. It happens before
> the "newDef" is created (for persistent vms) and before the implicit
> virDomainSaveStatus thus everything will work as it should.
>
> Took me a while to realize that though.
>
> I'd like to see a version that touches the documentation before giving
> my final word though.
>
> Also possibly a second opinion on the change of the behavior is welcome too.

Okay, I see your point. I think we have two other options here:

1) document that the file is left behind for transient domains

2) introduce yet another XML attribute to <nvram/> which would tell what 
to do with the NVRAM file on domain destroy (for transient domains).

Frankly, the more I think about this the more I like option 1). It's 
possible to leave a file behind even for persistent domains. Just start 
it with blank <nvram/>, let libvirt create the file for you, destroy the 
domain, and just before undefying it change domain XML so that no OVMF 
is configured for the domain.

Having said that, I believe my question is: does anybody object 
documenting the fact that NVRAM file may be left behind and it's mgmt 
app responsibility to remove the file once not needed?

Michal