Re: [PATCH] #4: Change interface between xen_unified & its underlying drivers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jul 04, 2007 at 05:24:03AM +0100, Daniel P. Berrange wrote:
> On Tue, Jul 03, 2007 at 04:19:13PM +0100, Richard W.M. Jones wrote:
> > 
> > This patch starts by removing the id, name and version fields from
> > virDriver.
> > 
> > It also removes getMaxVcpus and the domainLookup* fields, which will
> > make more sense when you see patches #6 and #7 in this series.
> 
> Yes, i guess 4, 6 & 7 should really be all one patch - since if you
> apply , but not 6 & 7 the driver is non-functional. Anyway, the combo
> of this & the other patches look like they're doing what I'd expect,
> so objections here...

  Honnestly I'm not really convinced as such the patches are an improvement.
You introduce a new structure, it's still indirect, how is that better ?
Now if you want to get rid of the indirection, why not but:
   - this forces to make all those functions public again
   - puts more logic in xen_unified, I'm not sure its code would
     become more maintainable
This would probably help in debugging sessions, that's right.
Do we want to do this while Xen support is still in flux, we need
to do Xen-API support (on localhost, I would not extend over network)
in a not so distant future, the driver based approach simplifies the 
integration of new code.

  I'm not against applying those 3 patches but at the moment I still
feel unconvinced :-)

Daniel

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard      | virtualization library  http://libvirt.org/
veillard@xxxxxxxxxx  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/

--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]