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

[Pavel Hrdina]

On Tue, Mar 22, 2016 at 10:06:19AM +0100, Jiri Denemark wrote:
> 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

Nice catch, you're right and this modification fixes the startup of new machine.

Thanks,

Pavel