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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 12/02/2015 05:13 PM, Martin Kletzander wrote:
On Wed, Dec 02, 2015 at 04:46:20PM +0800, lhuang wrote:


On 12/02/2015 01:35 AM, 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(-)



I am glad to see these patches you told me before.

And i have test your patches and found some problem until now:

1. if guest xml have private info, libvirt will output it if xml is invalid:

# virsh list --all --reason
Id    Name                           State      Reason
------------------------------------------------------------

-     test3                          shut off   invalid XML

# virsh -r dumpxml test3
...
   <input type='keyboard' bus='ps3'/>   <----wrong place

It should be in the same place as it was saved on the disk.

oh, seems you misunderstood, i mean there is the invalid place which make xml invalid :)


   <graphics type='vnc' port='-1' autoport='yes' listen='127.0.0.1'
passwd='123'>  <----show passwd


Oh, that's definitely what's not suppose to happen, but since we cannot
parse it, I will just disable the dumpxml for read-only connections.


okay, sounds good.

2. vcpucount command output looks strange (small problem :) ):

# virsh vcpucount test3


Oh, so my guess was that this happens because virsh calls dumpxml, then
searches for the info.  But I checked and it calls an API that should be
blocked.  I'm guessing that happened because of the uuid parsing has
gone wrong (below).  Anyway, I mentioned the first idea because that can
happen with something else and hence the only way how to properly
prevent that, is to use different API for invalid domains.  I'll try
working on that, but this is starting to be bigger and bigger overkill,
unfortunately.


I checked the code and found virshCPUCountCollect will return -1:

    if (checkState &&
        ((flags & VIR_DOMAIN_AFFECT_LIVE && virDomainIsActive(dom) < 1) ||
         (flags & VIR_DOMAIN_AFFECT_CONFIG && virDomainIsPersistent(dom) < 1)))
        return -1;

and PRINT_COUNT will skip which var <= 0, so command return success and no output.

Anyway, thanks a lot for testing that and letting me know.


You're welcome, i will reply this mail if i can find more problem.


Luyao


3. hit crash if the guest xml uuid is invalid (delete a number) during
stop daemon:


Program terminated with signal 11, Segmentation fault.
#0  0x00007fcbe773af87 in virDomainDefFree (def=0x7fcbc423d980) at
conf/domain_conf.c:2327
2327            virDomainGraphicsDefFree(def->graphics[i]);

Thread 1 (Thread 0x7fcbe835c8c0 (LWP 21589)):
#0  0x00007fcbe773af87 in virDomainDefFree (def=0x7fcbc423d980) at
conf/domain_conf.c:2327
#1  0x00007fcbe773b903 in virDomainObjDispose (obj=0x7fcbc4235fe0) at
conf/domain_conf.c:2487
#2  0x00007fcbe76f182b in virObjectUnref (anyobj=<optimized out>) at
util/virobject.c:265
#3  0x00007fcbe76d0ac9 in virHashFree (table=0x7fcbc412eda0) at
util/virhash.c:318
#4  0x00007fcbe76f182b in virObjectUnref (anyobj=<optimized out>) at
util/virobject.c:265
#5  0x00007fcbce85f8cf in qemuStateCleanup () at qemu/qemu_driver.c:1126
#6  0x00007fcbe779a168 in virStateCleanup () at libvirt.c:815
#7  0x00007fcbe83f02b1 in main (argc=<optimized out>, argv=<optimized
out>) at libvirtd.c:1619

Thanks,
Luyao

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list


--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]