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

[Daniel P. Berrange]

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.

> So my question to you is: do you have a better idea for this?
> 
> Many thanks in advance.
> 
> PS. I hope it's okay for you that I'm writing to you only for the moment
> (as it's somehow off topic). If not I'll send it to the mailing list :)

It isn't off-topic, so I'm copying back in the list.

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