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 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list