Re: [libvirt] [PATCH v2 00/10] Make loading domains with invalid XML possible

On 12/01/2015 12:35 PM, 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 --reason > Id Name State Reason > --------------------------------------------------------------- > - dummy shut off invalid XML > > # 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. > > v2: > - rebased on top of current master (with virdomainobjlist.c) > - only disallow starting domains with invalid definitions (change > done in v1), but allow re-defining them > - add support for "virsh list --reason" to better excercise the > feature added in this patchset > > v1: > - rebase > - don't allow starting domains with invalid state > - https://www.redhat.com/archives/libvir-list/2015-November/msg01127.html > > RFC: https://www.redhat.com/archives/libvir-list/2015-September/msg00698.html > > > Martin Kletzander (10): > conf, virsh: Add new domain shutoff reason > virsh: Refactor the table output of the list command > virsh: Add support for list -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, but don't allow > starting them > 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 | 121 +++++++++++++++++++++++++++++++-------- > src/conf/domain_conf.h | 7 +++ > src/conf/virdomainobjlist.c | 71 +++++++++++++++++++++-- > src/conf/virdomainobjlist.h | 1 + > src/libvirt_private.syms | 1 + > src/libxl/libxl_driver.c | 3 + > src/lxc/lxc_driver.c | 3 + > src/qemu/qemu_driver.c | 64 +++++++++++++++++---- > src/uml/uml_driver.c | 2 + > tests/virshtest.c | 30 +++++++--- > tools/virsh-domain-monitor.c | 60 ++++++++++++------- > tools/virsh.pod | 5 +- > 14 files changed, 303 insertions(+), 69 deletions(-) > > -- > 2.6.3 > > -- > libvir-list mailing list > libvir-list(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list > Beyond the few noted spots changes look good to me. Implicit ACK for those not specifically noted.

John