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

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 :-)

> > > 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);

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

> > 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.