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

[Martin Kletzander]

On Thu, Dec 10, 2015 at 03:20:20PM -0500, John Ferlan wrote:
>
>
>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 at 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.
>

Thanks a lot, but there is still the security issue and a crash
mentioned by Luyao.  I know how to deal with only a part of it.  Anyway,
this will need another version, so I'll include all the nits pointed out
in there, but it'll take some time again because from my POV this is
just a nice-to-have feature and nobody is asking for this, so there are
other, more pressing, things in the priority queue and hence this one
will have to wait again.  Anyway, thanks again for checking this out.

>John
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20151211/08c04302/attachment-0001.sig>