[libvirt] [RFC PATCH 0/8] Make loading domains with invalid XML possible

[Michal Privoznik]

On 22.09.2015 14:15, Martin Kletzander wrote:
> We always had to deal with new parsing errors in a weird way.  All of
> them needed to go into functions starting the domains.  That messes up
> the code, it's confusing to newcomers and so on.
> 
> I once had an idea that we can handle this stuff.  We know what
> failed, we have the XML that failed parsing and if the problem is not
> in the domain name nor UUID, we can create a domain object out of that
> as well.  This way we are able to do something we weren't with this
> series applied.  Example follows.
> 
> Assume "dummy" is a domain with invalid XML (I just modified the
> file).  Now, just for the purpose of this silly example, let's say we
> allowed domains with archtecture x86_*, but now we've realized that
> x86_64 is the only one we want to allow, but there already is a domain
> defined that has <type arch='x86_256' .../>.  With the current master,
> the domain would be lost, so we would need to modify the funstion
> starting the domain (e.g. qemuProcessStart for qemu domain).  However,
> with this series, it behaves like this:
> 
>   # virsh list --all
>    Id    Name                           State
>   ----------------------------------------------------
>    -     dummy                          shut off
> 
>   # virsh domstate --reason dummy
>   shut off (invalid XML)
> 
>   # virsh start dummy
>   error: Failed to start domain dummy
>   error: XML error: domain 'dummy' was not loaded due to an XML error
>   (unsupported configuration: Unknown architecture x86_256), please
>   redefine it
> 
>   # VISUAL='sed -i s/x86_256/x86_64/' virsh edit dummy
>   Domain dummy XML configuration edited.
> 
>   # virsh domstate --reason dummy
>   shut off (unknown)
> 
>   # virsh start dummy
>   Domain dummy started
> 
> This is a logical next step for us to clean and separate parsing and
> starting, getting rid of some old code without sacrifying compatibility
> and maybe generating parser code in the future.
> 
> Why is this an RFC?
>  - I can certainly easily imagine some people who might not like this
>  - It doesn't work for all drivers (yet)
>  - It does handle status XMLs, but only slightly.  All the handling is
>    done in a commit message that says something along the lines of
>    "beware, we should still be careful, but not as much as before".
>  - The error type used for the message is one that is already used for
>    something else and we might want a new type of error for exactly
>    this one error message, although it might be enough for some to
>    have the possibility of checking this by querying the domain state
>    and checking the reason.
>  - Just so you know I came up with something new for which there's no
>    BZ (or at least yet) =)
> 
> 
> Martin Kletzander (8):
>   conf, virsh: Add new domain shutoff reason
>   qemu: Few whitespace cleanups
>   conf: Extract name-parsing into its own function
>   conf: Extract UUID parsing into its own function
>   conf: Optionally keep domains with invalid XML
>   qemu: Don't lookup invalid domains unless specified otherwise
>   qemu: Prepare basic APIs to handle invalid defs
>   qemu: Load domains with invalid XML on start
> 
>  include/libvirt/libvirt-domain.h |   2 +
>  src/bhyve/bhyve_driver.c         |   2 +
>  src/conf/domain_conf.c           | 180 +++++++++++++++++++++++++++++++++------
>  src/conf/domain_conf.h           |   5 ++
>  src/libxl/libxl_driver.c         |   3 +
>  src/lxc/lxc_driver.c             |   3 +
>  src/qemu/qemu_driver.c           |  64 +++++++++++---
>  src/uml/uml_driver.c             |   2 +
>  tools/virsh-domain-monitor.c     |   3 +-
>  9 files changed, 223 insertions(+), 41 deletions(-)

So now that we have an agreement here on the design, I went through the
patches and they basically look okay. Except for a few places I've
pointed out. ACK series then except for 7/8 where some explanation is
needed.

Michal