[libvirt] [PATCH 3/3] networking API for hostonly networks in VirtualBox driver in libvirt

[Daniel Veillard]

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 at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/