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

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 ?

Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|