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);
The idea is to use 'netmask' to mask off the low bits in 'netaddr',
and then check the the remaining bits in 'netaddr' match those in
the requested 'address'.
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.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|