[libvirt] [PATCH 0/2] qemu: Small fix and validate the domain that actually will be started
Marc Hartmayer (2): qemu: Fix incorrect jump labels in error paths qemu: Validate the domain after marking the current domain as transient src/qemu/qemu_process.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) -- 2.5.5
Fix incorrect jump labels in error paths as the stop jump is only needed if the driver has already changed the state. For example 'virAtomicIntInc(&driver->nactive)' will be 'reverted' in the qemuProcessStop call. Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- src/qemu/qemu_process.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ea10fff..a57d136 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4755,9 +4755,12 @@ qemuProcessInit(virQEMUDriverPtr driver, */ VIR_DEBUG("Setting current domain def as transient"); if (virDomainObjSetDefTransient(caps, driver->xmlopt, vm) < 0) - goto stop; + goto cleanup; - if (!(flags & VIR_QEMU_PROCESS_START_PRETEND)) { + if (flags & VIR_QEMU_PROCESS_START_PRETEND) { + if (qemuDomainSetPrivatePaths(driver, vm) < 0) + goto cleanup; + } else { vm->def->id = qemuDriverAllocateID(driver); qemuDomainSetFakeReboot(driver, vm, false); virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_STARTING_UP); @@ -4770,10 +4773,10 @@ qemuProcessInit(virQEMUDriverPtr driver, VIR_HOOK_QEMU_OP_PREPARE, VIR_HOOK_SUBOP_BEGIN) < 0) goto stop; - } - if (qemuDomainSetPrivatePaths(driver, vm) < 0) - goto cleanup; + if (qemuDomainSetPrivatePaths(driver, vm) < 0) + goto stop; + } ret = 0; -- 2.5.5
On 02/23/2017 10:44 AM, Marc Hartmayer wrote:
Fix incorrect jump labels in error paths as the stop jump is only needed if the driver has already changed the state. For example 'virAtomicIntInc(&driver->nactive)' will be 'reverted' in the qemuProcessStop call.
Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- src/qemu/qemu_process.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ea10fff..a57d136 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4755,9 +4755,12 @@ qemuProcessInit(virQEMUDriverPtr driver, */ VIR_DEBUG("Setting current domain def as transient"); if (virDomainObjSetDefTransient(caps, driver->xmlopt, vm) < 0) - goto stop; + goto cleanup;
- if (!(flags & VIR_QEMU_PROCESS_START_PRETEND)) { + if (flags & VIR_QEMU_PROCESS_START_PRETEND) { + if (qemuDomainSetPrivatePaths(driver, vm) < 0) + goto cleanup;
This should have been goto stop; After SetDefTransien() succeeds, everything must goto stop in order to call qemuProcessStop which undoes the SetDefTransient(). I will fix it before pushing.
+ } else { vm->def->id = qemuDriverAllocateID(driver); qemuDomainSetFakeReboot(driver, vm, false); virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_STARTING_UP); @@ -4770,10 +4773,10 @@ qemuProcessInit(virQEMUDriverPtr driver, VIR_HOOK_QEMU_OP_PREPARE, VIR_HOOK_SUBOP_BEGIN) < 0) goto stop; - }
- if (qemuDomainSetPrivatePaths(driver, vm) < 0) - goto cleanup; + if (qemuDomainSetPrivatePaths(driver, vm) < 0) + goto stop; + }
ret = 0;
ACKed and pushed. Michal
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:
Fix incorrect jump labels in error paths as the stop jump is only needed if the driver has already changed the state. For example 'virAtomicIntInc(&driver->nactive)' will be 'reverted' in the qemuProcessStop call.
Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- src/qemu/qemu_process.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ea10fff..a57d136 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4755,9 +4755,12 @@ qemuProcessInit(virQEMUDriverPtr driver, */ VIR_DEBUG("Setting current domain def as transient"); if (virDomainObjSetDefTransient(caps, driver->xmlopt, vm) < 0) - goto stop; + goto cleanup;
- if (!(flags & VIR_QEMU_PROCESS_START_PRETEND)) { + if (flags & VIR_QEMU_PROCESS_START_PRETEND) { + if (qemuDomainSetPrivatePaths(driver, vm) < 0) + goto cleanup;
This should have been goto stop; After SetDefTransien() succeeds, everything must goto stop in order to call qemuProcessStop which undoes the SetDefTransient(). I will fix it before pushing.
Hmm why? In qemuProcessStop() the driver->nactive will be decreased and I think that is definitely not what we want if we haven't increased that value... (for the VIR_QEMU_PROCESS_START_PRETEND path)
+ } else { vm->def->id = qemuDriverAllocateID(driver); qemuDomainSetFakeReboot(driver, vm, false); virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_STARTING_UP); @@ -4770,10 +4773,10 @@ qemuProcessInit(virQEMUDriverPtr driver, VIR_HOOK_QEMU_OP_PREPARE, VIR_HOOK_SUBOP_BEGIN) < 0) goto stop; - }
- if (qemuDomainSetPrivatePaths(driver, vm) < 0) - goto cleanup; + if (qemuDomainSetPrivatePaths(driver, vm) < 0) + goto stop; + }
ret = 0;
ACKed and pushed.
Michal
-- 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
On 02/23/2017 04:57 PM, Marc Hartmayer wrote:
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:
Fix incorrect jump labels in error paths as the stop jump is only needed if the driver has already changed the state. For example 'virAtomicIntInc(&driver->nactive)' will be 'reverted' in the qemuProcessStop call.
Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- src/qemu/qemu_process.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ea10fff..a57d136 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4755,9 +4755,12 @@ qemuProcessInit(virQEMUDriverPtr driver, */ VIR_DEBUG("Setting current domain def as transient"); if (virDomainObjSetDefTransient(caps, driver->xmlopt, vm) < 0) - goto stop; + goto cleanup;
- if (!(flags & VIR_QEMU_PROCESS_START_PRETEND)) { + if (flags & VIR_QEMU_PROCESS_START_PRETEND) { + if (qemuDomainSetPrivatePaths(driver, vm) < 0) + goto cleanup;
This should have been goto stop; After SetDefTransien() succeeds, everything must goto stop in order to call qemuProcessStop which undoes the SetDefTransient(). I will fix it before pushing.
Hmm why? In qemuProcessStop() the driver->nactive will be decreased and I think that is definitely not what we want if we haven't increased that value... (for the VIR_QEMU_PROCESS_START_PRETEND path)
Ah, that's a good point. On the other hand, we need to call virDomainObjRemoveTransientDef(). So I guess the proper solution is to: if (qemuDomainSetPrivatePaths(driver, vm) < 0) { virDomainObjRemoveTransientDef(vm); goto cleanup; } I'll post the patch shortly. Michal
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; + if (flags & VIR_QEMU_PROCESS_START_PRETEND) { if (qemuDomainSetPrivatePaths(driver, vm) < 0) goto cleanup; -- 2.5.5
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). 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. Michal
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
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@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).
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/ :|
On Thu, Feb 23, 2017 at 05:36 PM +0100, "Daniel P. Berrange" <berrange@redhat.com> 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@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).
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.
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/ :|
-- 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
On Mon, Feb 27, 2017 at 10:59:39AM +0100, Marc Hartmayer wrote:
On Thu, Feb 23, 2017 at 05:36 PM +0100, "Daniel P. Berrange" <berrange@redhat.com> 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@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).
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.
There's no need to changes in the QEMU driver todo this. You can just query the current XML config, change the boot device in it, and then run virDomainCreateXML to launch the guest with the changed config, or virDomainDefineXML again to make the changed boot device permanent. 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/ :|
On Thu, Feb 23, 2017 at 17:22:44 +0100, Marc Hartmayer wrote:
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).
That does not make much sense. If you are changing the definition the code doing so should make sure that it does not create an invalid configuration rather than depending on the validation code.
participants (4)
-
Daniel P. Berrange -
Marc Hartmayer -
Michal Privoznik -
Peter Krempa