On Wed, May 06, 2009 at 06:12:21PM +0200, Pritesh Kothari wrote:
Hi All,
As discussed on the list resending the networking patch's. the patch's are as below: [PATCH 3/3]: contains networking API for hostonly networks in VirtualBox driver in libvirt (it contains all the fix's proposed on list along with two extra *DefinedNetworks functions)
Hum, I have a doubt here:
+/** + * The Network Functions here on + */ +static virDrvOpenStatus vboxNetworkOpen(virConnectPtr conn, + virConnectAuthPtr auth ATTRIBUTE_UNUSED, + int flags ATTRIBUTE_UNUSED) { + vboxGlobalData *data = conn->privateData; [...] + DEBUG0("network intialized"); + /* conn->networkPrivateData = some network specific data */ + return VIR_DRV_OPEN_SUCCESS; [...] +static int vboxNetworkClose(virConnectPtr conn) { + DEBUG0("network unintialized"); + conn->networkPrivateData = NULL; + return 0; +}
You really don't need to keep any data about the networking driver in libvirt(d) itself ? [...]
+ /* Currently support only one dhcp server per network + * with contigious address space from start to end + */ + if ((def->nranges == 1) && + (def->ranges[0].start) && + (def->ranges[0].end)) {
Hum, what about at least allowing the first range if multiple were defined instead of disabling DHCP completely. I would suggest to start with if ((def->nranges >= 1) && instead. [...]
+ /* Currently support only one dhcp server per network + * with contigious address space from start to end + */ + if ((def->nranges == 1) && + (def->ranges[0].start) && + (def->ranges[0].end)) {
Same here. The patch is large but reuses the common routines for conversion to/from XML, a lot of the code is vbox internal structures peek/poke which are a bit hard to follow but this looks regular, the patch looks fine to me but feedback on previous points would be appreciated :-) Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/