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

[Marc Hartmayer]

On Wed, Mar 08, 2017 at 10:41 AM +0100, "Daniel P. Berrange" <berrange at redhat.com> wrote:
> On Wed, Mar 08, 2017 at 10:32:32AM +0100, Marc Hartmayer wrote:
>> On Mon, Feb 27, 2017 at 11:20 AM +0100, "Daniel P. Berrange" <berrange at redhat.com> wrote:
>> > 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 at 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 at 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 at linux.vnet.ibm.com>
>> >> >> >> Reviewed-by: Bjoern Walk <bwalk at linux.vnet.ibm.com>
>> >> >> >> Reviewed-by: Boris Fiuczynski <fiuczy at 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.
>> >
>> 
>> First of all, I hope I understand you right :) (you can tell me as soon
>> as you have read the following text)
>> 
>> I've followed your advice and tried to add this functionality without
>> any changes in the QEMU/hypervisor. For this I've created a new function
>> in the remote driver which will call the needed functions (get current
>> XML config for the domain, manipulate the XML config, and then use
>> virDomainCreateXML for starting).
>> 
>> To get the current XML config is straightforward as you've mentioned -
>> just use 'virDomanGetXMLDesc(...)'.
>> 
>> But how can I change the boot device?
>> 1) I could use libxml2 for manipulating the XML string which we will get
>> with calling 'virDomainGetXMLDesc(..)' but this is error-prone and just
>> duplicate work as we have to parse the XML string and the information of
>> it. Also, if there are any additions in the future for boot devices
>> there will be no 'compile time reminder' that you have to edit this
>> function.
>
> Yes, this is the way it is normally done. There is a tradeoff here between
> wanting to be reminded at compile time but seeing build breakage, and libvirt
> wanting to be able to extend its config info without breaking apps. The
> libvirt view is that the latter is more important.
>
>> or
>> 2) Parse the XML string into our internal representation (virDomainDef),
>> adjust the boot orders and create a new XML string out of this internal
>> representation (reverse to the way how virDomainDefCopy(...) works). But
>> there is the huge problem:
>> 
>> We need 'virCapsPtr caps' and 'virDomainXMLOptionPtr xmlopt' for
>> virDomainDefFormat(...) and virDomainDefParseString(...) but we're on
>> the remote driver layer and therefore we've no direct access to this. We
>> could add a function to the hypervisor driver interface for getting
>> these but this is just awkward.
>
> The virDomainDef are private data structures just for internal libvirt
> usage. Even though virsh is in the libvirt source tree, it should be
> treated as if it were a standalone application and thus only use the
> public APIs.

I am trying to implement this in libvirtd as the s390 architecture has
1. only one boot device – there is no option to add something like
multiple boot devices and sort them with a boot order. 2. s390 has no
possibility to temporarily change the boot order during booting as it is
possible within x86 BIOS.  With that in mind the enablement to change
the boot device while starting a (transient) domain in libvirtd is a way
to enable s390 architecture to do something similar.

(snip)

-- 
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