On Thu, Feb 23, 2017 at 03:33 PM +0100, Michal Privoznik <mprivozn@redhat.com> 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@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- 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).
However, the StartValidate() just reads some data without any allocation. Thus, from memory management POV, should your domain be unable to start we don't allocate any memory just to find that out.
Is this point that important? It's likely that our 'virDomainObjSetDefTransient -> virDomainDefCopy -> virDomainDefFormat and virDomainDefParseString' does something wrong and this way we could detect this. -- Beste Grüße / Kind regards Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294