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.

> I thought about your approach as well, but that would require complete > rewrite of the whole parsing code (maybe changing it to generated one > based on RNG schema and some minor additional info) and it would touch > way more APIs. The approach I'm suggesting here keeps almost all APIs > untouched and it doesn't have an effect on anything, unless explicitly > specified. > > Check last two patches to see how much qemu driver needs to be > changed, for others it will be similar, probably a little bit less.. > The patches are 17+/9- when combined. I don't think you can do much > better than that. And that doesn't show the improvements that can be > made in the starting and parsing code after this series is applied. 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 :|