Re: [libvirt] [PATCH 2/2] qemu: Validate the domain after marking the current domain as transient

On Thu, Feb 23, 2017 at 05:36 PM +0100, "Daniel P. Berrange" <berrange(a)redhat.com&gt; wrote: > On Thu, Feb 23, 2017 at 05:22:44PM +0100, Marc Hartmayer wrote: >> On Thu, Feb 23, 2017 at 03:33 PM +0100, Michal Privoznik <mprivozn(a)redhat.com&gt; wrote: >> > On 02/23/2017 10:44 AM, Marc Hartmayer wrote: >> >> Validate the domain that actually will be started. It's semantically >> >> more clear and also it can detect failures that may have happened in >> >> virDomainObjSetDefTransient(). >> >> >> >> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com&gt; >> >> Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com&gt; >> >> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; >> >> --- >> >> src/qemu/qemu_process.c | 6 +++--- >> >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> >> >> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c >> >> index a57d136..bd3a8b8 100644 >> >> --- a/src/qemu/qemu_process.c >> >> +++ b/src/qemu/qemu_process.c >> >> @@ -4746,9 +4746,6 @@ qemuProcessInit(virQEMUDriverPtr driver, >> >> vm->def->os.machine))) >> >> goto cleanup; >> >> >> >> - if (qemuProcessStartValidate(driver, vm, priv->qemuCaps, caps, flags) < 0) >> >> - goto cleanup; >> >> - >> >> /* Do this upfront, so any part of the startup process can add >> >> * runtime state to vm->def that won't be persisted. This let's us >> >> * report implicit runtime defaults in the XML, like vnc listen/socket >> >> @@ -4757,6 +4754,9 @@ qemuProcessInit(virQEMUDriverPtr driver, >> >> if (virDomainObjSetDefTransient(caps, driver->xmlopt, vm) < 0) >> >> goto cleanup; >> >> >> >> + if (qemuProcessStartValidate(driver, vm, priv->qemuCaps, caps, flags) < 0) >> >> + goto cleanup; >> >> + >> > >> > This needs to be goto stop for the reasons described in the previous e-mail. >> > >> >> if (flags & VIR_QEMU_PROCESS_START_PRETEND) { >> >> if (qemuDomainSetPrivatePaths(driver, vm) < 0) >> >> goto cleanup; >> >> >> > >> > Honestly, I like what we have now better. I mean, SetDefTransient() is >> > very unlikely to fail. It's just doing a copy of domain definition (in a >> > very stupid way, but lets save that for a different discussion). >> > Basically, it will fail on OOM only (which you will not get on a Linux >> > system, unless you really try). >> >> It's semantically more clear (at least for me) and for example it >> enables us to change some parts of the transient domain before >> validation (affect the transient domain only, not the persistent). > > What are you planning to change in the config before validation ? > I'm implementing a feature which allows to select the boot device at domain start time (something like 'virsh start --with-bootdevice sda domain1'). The main reason why we want this is because the s390 architecture boots only from the first device specified in the boot order.