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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list