[libvirt] [PATCH 02/12] nvram: generate it's path in qemuDomainDefPostParse

[Jiri Denemark]

On Tue, Mar 15, 2016 at 14:15:58 +0100, Pavel Hrdina wrote:
> The postParse callback is the correct place to generate default values
> that should be present in offline XML.
> 
> Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
> ---
>  src/qemu/qemu_domain.c  |  10 ++++
>  src/qemu/qemu_process.c | 153 ++++++++++++++++++------------------------------
>  2 files changed, 68 insertions(+), 95 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 594063e..632cf47 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1397,6 +1397,7 @@ qemuDomainDefPostParse(virDomainDefPtr def,
>                         void *opaque)
>  {
>      virQEMUDriverPtr driver = opaque;
> +    virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>      virQEMUCapsPtr qemuCaps = NULL;
>      int ret = -1;
>  
> @@ -1412,6 +1413,15 @@ qemuDomainDefPostParse(virDomainDefPtr def,
>          return ret;
>      }
>  
> +    if (def->os.loader &&
> +        def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH &&
> +        def->os.loader->readonly == VIR_TRISTATE_SWITCH_ON &&
> +        !def->os.loader->nvram) {
> +        if (virAsprintf(&def->os.loader->nvram, "%s/%s_VARS.fd",
> +                        cfg->nvramDir, def->name) < 0)
> +            goto cleanup;
> +    }
> +
>      /* check for emulator and create a default one if needed */
>      if (!def->emulator &&
>          !(def->emulator = virDomainDefGetDefaultEmulator(def, caps)))
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 3c496cb..958fae3 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3878,7 +3878,6 @@ qemuProcessVerifyGuestCPU(virQEMUDriverPtr driver,
>  
>  static int
>  qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
> -                 virCapsPtr caps,
>                   virDomainObjPtr vm,
>                   bool migrated)
>  {
> @@ -3886,111 +3885,81 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
>      int srcFD = -1;
>      int dstFD = -1;
>      virDomainLoaderDefPtr loader = vm->def->os.loader;
> -    bool generated = false;
>      bool created = false;
> +    const char *master_nvram_path;
> +    ssize_t r;
>  
> -    /* Unless domain has RO loader of pflash type, we have
> -     * nothing to do here.  If the loader is RW then it's not
> -     * using split code and vars feature, so no nvram file needs
> -     * to be created. */
> -    if (!loader || loader->type != VIR_DOMAIN_LOADER_TYPE_PFLASH ||
> -        loader->readonly != VIR_TRISTATE_SWITCH_ON)
> +    if (!loader->nvram || !migrated)
>          return 0;
>  
> -    /* If the nvram path is configured already, there's nothing
> -     * we need to do. Unless we are starting the destination side
> -     * of migration in which case nvram is configured in the
> -     * domain XML but the file doesn't exist yet. Moreover, after
> -     * the migration is completed, qemu will invoke a
> -     * synchronization write into the nvram file so we don't have
> -     * to take care about transmitting the real data on the other
> -     * side. */
> -    if (loader->nvram && !migrated)
> +    if (virFileExists(loader->nvram))
>          return 0;

So previously if loader->nvram != NULL, the file was already created
(except for migration). Now you fixed it and generated nvram path has no
relation to the existence of the file. It doesn't make any sense to
check whether we are migrating or not anymore. I think the check should
be

    if (!loader->nvram || virFileExists(loader->nvram))
        return 0;

The check you have would only create the nvram file if loader->nvram !=
NULL and migrated = true, i.e., it won't be created if you start a new
domain without migrating it.

ACK to the patch if you agree with ^, otherwise an explanation of why
your current code is correct will be needed :-)

Jirka