On Mon, May 26, 2014 at 09:39:43PM +0200, Tom Gundersen wrote: > Make sure we don't hand out the same IP twice. We still don't > handle lease expiry. > --- > src/libsystemd-network/dhcp-server-internal.h | 25 ++++- > src/libsystemd-network/sd-dhcp-server.c | 136 > ++++++++++++++++++++++++-- > src/libsystemd-network/test-dhcp-server.c | 59 +++++++++++ > 3 files changed, 206 insertions(+), 14 deletions(-) > > diff --git a/src/libsystemd-network/dhcp-server-internal.h > b/src/libsystemd-network/dhcp-server-internal.h > index ce2e260..7fe7253 100644 > --- a/src/libsystemd-network/dhcp-server-internal.h > +++ b/src/libsystemd-network/dhcp-server-internal.h > @@ -23,12 +23,25 @@ > #include "sd-event.h" > #include "sd-dhcp-server.h" > > +#include "hashmap.h" > #include "refcnt.h" > #include "util.h" > #include "log.h" > > #include "dhcp-internal.h" > > +typedef struct DHCPClientId { > + size_t length; > + uint8_t *data; > +} DHCPClientId; > + > +typedef struct DHCPLease { > + DHCPClientId client_id; > + > + be32_t address; > + usec_t expiration; > +} DHCPLease; > + > struct sd_dhcp_server { > RefCount n_ref; > > @@ -42,12 +55,11 @@ struct sd_dhcp_server { > be32_t address; > be32_t pool_start; > size_t pool_size; > -}; > + size_t next_offer; > > -typedef struct DHCPClientId { > - size_t length; > - uint8_t *data; > -} DHCPClientId; > + Hashmap *leases_by_client_id; > + DHCPLease **bound_leases; > +}; > > typedef struct DHCPRequest { > /* received message */ > @@ -71,3 +83,6 @@ int dhcp_server_handle_message(sd_dhcp_server *server, > DHCPMessage *message, > int dhcp_server_send_packet(sd_dhcp_server *server, > DHCPRequest *req, DHCPPacket *packet, > int type, size_t optoffset); > + > +unsigned long client_id_hash_func(const void *p, const uint8_t > hash_key[HASH_KEY_SIZE]); > +int client_id_compare_func(const void *_a, const void *_b); > diff --git a/src/libsystemd-network/sd-dhcp-server.c > b/src/libsystemd-network/sd-dhcp-server.c > index 3de61f7..44ca645 100644 > --- a/src/libsystemd-network/sd-dhcp-server.c > +++ b/src/libsystemd-network/sd-dhcp-server.c > @@ -23,6 +23,8 @@ > #include <sys/ioctl.h> > #include <netinet/if_ether.h> > > +#include "siphash24.h" > + > #include "sd-dhcp-server.h" > #include "dhcp-server-internal.h" > #include "dhcp-internal.h" > @@ -37,6 +39,11 @@ int sd_dhcp_server_set_lease_pool(sd_dhcp_server *server, > struct in_addr *addres > assert_return(size, -EINVAL); > assert_return(server->pool_start == htobe32(INADDR_ANY), -EBUSY); > assert_return(!server->pool_size, -EBUSY); > + assert_return(!server->bound_leases, -EBUSY); > + > + server->bound_leases = new0(DHCPLease*, size); > + if (!server->bound_leases) > + return -ENOMEM; > > server->pool_start = address->s_addr; > server->pool_size = size; > @@ -62,13 +69,63 @@ sd_dhcp_server *sd_dhcp_server_ref(sd_dhcp_server > *server) { > return server; > } > > +unsigned long client_id_hash_func(const void *p, const uint8_t > hash_key[HASH_KEY_SIZE]) { > + uint64_t u; > + const DHCPClientId *id = p; > + > + assert(id); > + assert(id->length); > + assert(id->data); > + > + siphash24((uint8_t*) &u, id->data, id->length, hash_key); > + > + return (unsigned long) u; > +} > + > +int client_id_compare_func(const void *_a, const void *_b) { > + const DHCPClientId *a, *b; > + > + a = _a; > + b = _b; > + > + assert(!a->length || a->data); > + assert(!b->length || b->data); > + > + if (a->length != b->length) > + return a->length < b->length ? -1 : 1; > + > + return memcmp(a->data, b->data, a->length); > +} > + > +static void dhcp_lease_free(DHCPLease *lease) { > + if (!lease) > + return; > + > + free(lease->client_id.data); > + free(lease); > +} > + > +DEFINE_TRIVIAL_CLEANUP_FUNC(DHCPLease*, dhcp_lease_free); > +#define _cleanup_dhcp_lease_free_ _cleanup_(dhcp_lease_freep) > + > sd_dhcp_server *sd_dhcp_server_unref(sd_dhcp_server *server) { > if (server && REFCNT_DEC(server->n_ref) <= 0) { > + DHCPLease *lease; > + Iterator i; > + > log_dhcp_server(server, "UNREF"); > > sd_dhcp_server_stop(server); > > sd_event_unref(server->event); > + > + HASHMAP_FOREACH(lease, server->leases_by_client_id, i) { > + hashmap_remove(server->leases_by_client_id, lease); > + dhcp_lease_free(lease); > + } > + > + hashmap_free(server->leases_by_client_id); > + free(server->bound_leases); > free(server); > } > > @@ -90,6 +147,7 @@ int sd_dhcp_server_new(sd_dhcp_server **ret, int ifindex) { > server->fd = -1; > server->address = htobe32(INADDR_ANY); > server->index = ifindex; > + server->leases_by_client_id = hashmap_new(client_id_hash_func, > client_id_compare_func); > > *ret = server; > server = NULL; > @@ -478,9 +536,25 @@ static int ensure_sane_request(DHCPRequest *req, > DHCPMessage *message) { > return 0; > } > > +static int get_pool_offset(sd_dhcp_server *server, be32_t requested_ip) { > + assert(server); > + > + if (!server->pool_size) > + return -EINVAL; > + > + if (be32toh(requested_ip) < be32toh(server->pool_start) || > + be32toh(requested_ip) >= be32toh(server->pool_start) + > + + server->pool_size) > + return -EINVAL; > + > + return (be32toh(requested_ip) - > + - be32toh(server->pool_start)) % server->pool_size; This looks wrong, double minus comes out to a plus, and the modulo should not be necessary because of the check above.
> +} > + > int dhcp_server_handle_message(sd_dhcp_server *server, DHCPMessage *message, > size_t length) { > _cleanup_dhcp_request_free_ DHCPRequest *req = NULL; > + DHCPLease *existing_lease; > int type, r; > > assert(server); > @@ -504,10 +578,13 @@ int dhcp_server_handle_message(sd_dhcp_server *server, > DHCPMessage *message, > /* this only fails on critical errors */ > return r; > > + existing_lease = hashmap_get(server->leases_by_client_id, > &req->client_id); > + > switch(type) { > case DHCP_DISCOVER: > { > - be32_t address; > + be32_t address = INADDR_ANY; > + unsigned i; > > log_dhcp_server(server, "DISCOVER (0x%x)", > be32toh(req->message->xid)); > @@ -516,9 +593,22 @@ int dhcp_server_handle_message(sd_dhcp_server *server, > DHCPMessage *message, > /* no pool allocated */ > return 0; > > - /* for now pick a random address from the pool */ > - address = htobe32(be32toh(server->pool_start) + > - (random_u32() % server->pool_size)); Parentheses are not be necessary, modulo has the same precedence as multiplication. > + /* for now pick a random free address from the pool */ > + if (existing_lease) > + address = existing_lease->address; > + else { > + for (i = 0; i < server->pool_size; i++, > server->next_offer++) { > + if > (!server->bound_leases[(server->next_offer) % server->pool_size]) { Misleading parentheses here. > + address = > htobe32(be32toh(server->pool_start) + > + + > (server->next_offer) % server->pool_size); And here. > + break; Hm, maybe it would be nicer to apply the modulo already when incrementing the variable? Might be more readable that way: for (i = 0; i < server->pool_size; i++) if (!server->bound_leases[server->next_offer]) { address = htobe32(be32toh(server->pool_start) + server->next_offer); break; } else server->next_offer = (server->next_offer + 1) % server->pool_size; > + } > + } > + } > + > + if (address == INADDR_ANY) > + /* no free addresses left */ > + return 0; > > r = server_send_offer(server, req, address); > if (r < 0) { > @@ -537,6 +627,7 @@ int dhcp_server_handle_message(sd_dhcp_server *server, > DHCPMessage *message, > { > be32_t address; > bool init_reboot = false; > + int pool_offset; > > /* see RFC 2131, section 4.3.2 */ > > @@ -583,19 +674,46 @@ int dhcp_server_handle_message(sd_dhcp_server *server, > DHCPMessage *message, > address = req->message->ciaddr; > } > > - /* for now we just verify that the address is from the pool, > not > - whether or not it is taken */ > - if (htobe32(req->requested_ip) >= > htobe32(server->pool_start) && > - htobe32(req->requested_ip) < htobe32(server->pool_start) > + > - + server->pool_size) { > + pool_offset = get_pool_offset(server, address); > + > + /* verify that the requested address is from the pool, and > either > + owned by the current client or free */ > + if (pool_offset >= 0 && > + server->bound_leases[pool_offset] == existing_lease) { > + DHCPLease *lease; > + usec_t time_now; > + > + if (!existing_lease) { > + lease = new0(DHCPLease, 1); > + lease->address = req->requested_ip; > + lease->client_id.data = > memdup(req->client_id.data, > + > req->client_id.length); Indentation. > + if (!lease->client_id.data) > + return -ENOMEM; > + lease->client_id.length = > req->client_id.length; > + } else > + lease = existing_lease; > + > + r = sd_event_now(server->event, CLOCK_MONOTONIC, > &time_now); > + if (r < 0) > + time_now = now(CLOCK_MONOTONIC); > + lease->expiration = req->lifetime * USEC_PER_SEC + > time_now; > + > r = server_send_ack(server, req, address); > if (r < 0) { > log_dhcp_server(server, "could not send ack: > %s", > strerror(-r)); > + if (!existing_lease) > + dhcp_lease_free(lease); > + > return 0; > } else { > log_dhcp_server(server, "ACK (0x%x)", > be32toh(req->message->xid)); > + > + server->bound_leases[pool_offset] = lease; > + hashmap_put(server->leases_by_client_id, > &lease->client_id, lease); > + > return DHCP_ACK; > } > } else if (init_reboot) { > diff --git a/src/libsystemd-network/test-dhcp-server.c > b/src/libsystemd-network/test-dhcp-server.c > index ea256b9..369e59f 100644 > --- a/src/libsystemd-network/test-dhcp-server.c > +++ b/src/libsystemd-network/test-dhcp-server.c > @@ -90,6 +90,11 @@ static void test_message_handler(void) { > uint8_t length; > be32_t address; > } _packed_ option_server_id; > + struct { > + uint8_t code; > + uint8_t length; > + uint8_t id[7]; > + } _packed_ option_client_id; > uint8_t end; > } _packed_ test = { > .message.op = BOOTREQUEST, > @@ -156,14 +161,67 @@ static void test_message_handler(void) { > test.option_server_id.address = htobe32(INADDR_LOOPBACK); > test.option_requested_ip.address = htobe32(INADDR_LOOPBACK + 3); > assert_se(dhcp_server_handle_message(server, (DHCPMessage*)&test, > sizeof(test)) == DHCP_ACK); > + > test.option_server_id.address = htobe32(0x12345678); > test.option_requested_ip.address = htobe32(INADDR_LOOPBACK + 3); > assert_se(dhcp_server_handle_message(server, (DHCPMessage*)&test, > sizeof(test)) == 0); > test.option_server_id.address = htobe32(INADDR_LOOPBACK); > + test.option_requested_ip.address = htobe32(INADDR_LOOPBACK + 4); > + assert_se(dhcp_server_handle_message(server, (DHCPMessage*)&test, > sizeof(test)) == 0); > + test.option_requested_ip.address = htobe32(INADDR_LOOPBACK + 3); > + assert_se(dhcp_server_handle_message(server, (DHCPMessage*)&test, > sizeof(test)) == DHCP_ACK); > + > + test.option_client_id.code = DHCP_OPTION_CLIENT_IDENTIFIER; > + test.option_client_id.length = 7; > + test.option_client_id.id[0] = 0x01; > + test.option_client_id.id[1] = 'A'; > + test.option_client_id.id[2] = 'B'; > + test.option_client_id.id[3] = 'C'; > + test.option_client_id.id[4] = 'D'; > + test.option_client_id.id[5] = 'E'; > + test.option_client_id.id[6] = 'F'; > + assert_se(dhcp_server_handle_message(server, (DHCPMessage*)&test, > sizeof(test)) == DHCP_ACK); > + > test.option_requested_ip.address = htobe32(INADDR_LOOPBACK + 30); > assert_se(dhcp_server_handle_message(server, (DHCPMessage*)&test, > sizeof(test)) == 0); > } > > +static void test_client_id_hash(void) { > + DHCPClientId a = { > + .length = 4, > + }, b = { > + .length = 4, > + }; > + uint8_t hash_key[HASH_KEY_SIZE] = { > + '0', '1', '2', '3', '4', '5', '6', '7', > + '8', '9', 'A', 'B', 'C', 'D', 'E', 'F', > + }; > + > + a.data = (uint8_t*)strdup("abcd"); > + b.data = (uint8_t*)strdup("abcd"); > + > + assert_se(client_id_compare_func(&a, &b) == 0); > + assert_se(client_id_hash_func(&a, hash_key) == > client_id_hash_func(&b, hash_key)); > + a.length = 3; > + assert_se(client_id_compare_func(&a, &b) != 0); > + a.length = 4; > + assert_se(client_id_compare_func(&a, &b) == 0); > + assert_se(client_id_hash_func(&a, hash_key) == > client_id_hash_func(&b, hash_key)); > + > + b.length = 3; > + assert_se(client_id_compare_func(&a, &b) != 0); > + b.length = 4; > + assert_se(client_id_compare_func(&a, &b) == 0); > + assert_se(client_id_hash_func(&a, hash_key) == > client_id_hash_func(&b, hash_key)); > + > + free(b.data); > + b.data = (uint8_t*)strdup("abce"); > + assert_se(client_id_compare_func(&a, &b) != 0); > + > + free(a.data); > + free(b.data); > +} > + > int main(int argc, char *argv[]) { > _cleanup_event_unref_ sd_event *e; > > @@ -175,6 +233,7 @@ int main(int argc, char *argv[]) { > > test_basic(e); > test_message_handler(); > + test_client_id_hash(); > > return 0; > } Zbyszek _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel