[libvirt] [PATCH] 2/3 Store the range size when adding a DHCP range

[Daniel Veillard]

On Wed, Oct 14, 2009 at 10:57:43AM +0100, Daniel P. Berrange wrote:
> On Tue, Oct 13, 2009 at 04:14:00PM +0200, Daniel Veillard wrote:
> > +
> > +        /*
> > +         * check at least the 2 first IP match i.e on same class C subnet
> > +         */
> > +        for (i = 0; i < 2;i++) {
> > +            if (ip4s[i] != ip4e[i]) {
> > +                virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR,
> > +                   _("start and end of DHCP range do not match '%s' and '%s'"),
> > +                                      start, end);
> > +                return(-1);
> > +            }
> > +        }
> 
> Shouldn't we be comparing each of DHCP addresses against the 'netmask'
> field we have in virNetworkDef instead. It'd be nice to have a separate
> function for this like

  As far as I can tell the netmask is optional. but if it's there, sure.

>     virSocketAddrInNetwork(struct sockaddr_storage *address,
>                            struct sockaddr_storage *netmask);

  Hum, I don't understand what that function would do suppose you have
1.2.3.4 and 255.255.255.0 what kind of thing can you do. Sure if you
pass 2 addresses then you can check they pertain to the same netmask
but that function signature can't work IMHO

> since there's a couple of other places we ought todo this kind of
> validation.
> 
> > +        ret = ip4e[3] - ip4s[3] + 256 * (ip4e[2] - ip4s[2]);
> 
> It would be nice to have this in a callable function too
> 
>      int virSocketAddrRange(struct sockaddr_storage *start,
>                             struct sockaddr_storage *end);

  Are you supposed to look struct sockaddr_storage ? As posted in my
last mail this seems a completely opaque structure at least in theory
and if you want to keep the portability it's supposed to bring.

> > +
> > +        /*
> > +         * a bit of sanity checking on the range
> > +         * Should we complain for a range of more than 10,000 ?
> > +         */
> 
> Its probably sufficient to leave dnsmasq to do validation.

  on the second point, yes, for start/end order it's probably easier to
capture and report immediately.  the Network parsing code is lax, it
reports errors but doesn't stop processing (except for allocation
failure) the definition, I assume the goal is to always have a dnsmasq
running even if some args are missing, hence it's better to make as much
checking before calling. If my assumption is incorrect then that code
should be made strict and return -1 on error instead.

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/