[libvirt] [RFC PATCH 7/8] qemu: Prepare basic APIs to handle invalid defs

[Daniel P. Berrange]

On Tue, Oct 06, 2015 at 03:35:36PM +0200, Michal Privoznik wrote:
> On 22.09.2015 15:17, Daniel P. Berrange wrote:
> > On Tue, Sep 22, 2015 at 03:12:14PM +0200, Martin Kletzander wrote:
> >> On Tue, Sep 22, 2015 at 01:48:01PM +0100, Daniel P. Berrange wrote:
> >>> On Tue, Sep 22, 2015 at 02:15:53PM +0200, Martin Kletzander wrote:
> >>>> In order for the user to be able to fix broken domains function
> >>>> qemuDomainGetXMLDesc() needs to be able to lookup invalid domain
> >>>> definitions and handle them properly.  When redefined, function
> >>>> qemuDomainDefineXMLFlags() must clear the 'invalid XML' reason.  As a
> >>>> nice addition, qemuDomainGetState() can lookup such domains without any
> >>>> other change and that allows virsh not only to get their status, but
> >>>> also to list them.
> >>>
> >>> Hmm, that's an interesting approach to the problem. I wonder though
> >>> if we could do things slightly differently such that we don't need
> >>> to change so many APIs.
> >>>
> >>> eg, just have a 'bool error' field in virDomainDefPtr. When loading
> >>> the XML fails, populate a virDomainObjPtr/DefPtr as normal, but set
> >>> the error field. Now we merely need to change the qemuDomainStart
> >>> method, so it refuses to launch a VM with the 'error' flag set. All
> >>> the other APIs could be essentially unchanged. Sure it would not
> >>> be useful to allow things like virDomainAttachDevice, etc on such
> >>> broken domains, but for sake of simplicity we can avoid touching
> >>> all the methods except start.
> >>>
> >>
> >> To be honest, I'm a afraid that we might forget some API that needs to
> >> be blocked as well.  And we would have to go through all the APIs just
> >> to see whether it accesses something that might be missing.  Moreover,
> >> how would you decide what to skip at each error during parsing?  If,
> >> for example, the <numatune> has some faulty attribute in one
> >> subelement should we skip all the elements or just the one or just
> >> skip that one particular attribute?  We would also not format the
> >> faulty attribute to the XML being dumped, so the user wouldn't see
> >> what's missing, which is even worse when there are two problems and
> >> they fix only one, so we skip the other one.
> > 
> > Oh, I wasn't suggesting changing the parser. I just mean that we would
> > create a virDomainDefPtr instance which only contains the name and
> > UUID, and ID field (if the guest is running from domain status XML).
> > 
> > We'd leave all the rest of the config blank - our code ought to be
> > able to deal with that, since essentially all config is optional
> > aside from the name/uuid anyway.
> 
> So if I understand it correctly, if there's an error during XML parsing,
> we would:
> 
> virDomainDefFree(def); /* becasue @def is parsed just partially */
> def = virDomainDefNewFull(name, uuid, -1);
> def->error = true; /* or something */
> 
> But where would be the original (invalid) XML be stored? We don't want
> to lost it. I rather prefer to giving it back to the user (in dumpXML
> api) and thus giving him chance to fix the problem that caused error in
> the first place. Otherwise this seems pointless to me.

Hmm, good point.

> And I don't expect that many APIs need fixing. It's just a few of them
> that should work with broken XML: dumpXML and probably GetState (so that
> we can report domain is shut off due to invalid XML).

I don't feel particularly strongly about the approach, so if you want
to go the route you originally described its fine.

BTW, I guess we want this for all object types, not just domains. ie
storage, network, etc.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|