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

[Peter Krempa]

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.


> 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.

Peter

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20141112/a37a05cb/attachment-0001.sig>