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

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