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

[Michal Privoznik]

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.

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).

Michal