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

[Daniel Veillard]

On Wed, Oct 14, 2009 at 12:33:36PM +0100, Daniel P. Berrange wrote:
> On Wed, Oct 14, 2009 at 12:59:53PM +0200, Daniel Veillard wrote:
> > 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.
> 
> Netmask is actually compulsory, although it shouldn't be since there are
> well defined defaults for both IPv4 & 6 :-)

  Then we need to fix the schemas !

docs/schemas/network.rng:

      <optional>
        <attribute name="netmask"><text/></attribute>
      </optional>

> > 
> > >     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
> 
> Opps, my mistake this should have had 3 args
> 
>  virSocketAddrInNetwork(struct sockaddr_storage *address,
>                         struct sockaddr_storage *netaddr,
>                         struct sockaddr_storage *netmask);
> 

  Ah, okay, that makes sense.

> We already have some horrible code in virNetworkDefParseXML()
> which can do the  netaddr+netmask  bit masking for IPv4, to
> calculate a network address. 

  if I have an array it's trivial. But the struct sockaddr_storage
opaque is weird.

> > > 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.
> 
> You cast to one of the address specific structs according
> to the ss_family field.

  humpf ... okay it has to be cast to be accessed, that's weird,
definitely.

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/