Re: [libvirt] [PATCH 3/3] networking API for hostonly networks in VirtualBox driver in libvirt
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/