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@xxxxxxxxxx>
> > ---
> > 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list