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

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.