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@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list