Discussion:
[systemd-devel] [PATCH 02/28] dhcp: Add DHCP client initialization
Patrik Flykt
2013-11-13 21:22:30 UTC
Permalink
Provide functionality for initializing a DHCP client struct, setting
interface index, last used address and additional options to request.
On initialization the most useful options are added by default.
---
src/dhcp/client.c | 137 +++++++++++++++++++++++++++++++++++++++++++++++++++++
src/dhcp/client.h | 34 +++++++++++++
2 files changed, 171 insertions(+)
create mode 100644 src/dhcp/client.c
create mode 100644 src/dhcp/client.h

diff --git a/src/dhcp/client.c b/src/dhcp/client.c
new file mode 100644
index 0000000..172ddd9
--- /dev/null
+++ b/src/dhcp/client.c
@@ -0,0 +1,137 @@
+/***
+ This file is part of systemd.
+
+ Copyright (C) 2013 Intel Corporation. All rights reserved.
+
+ systemd is free software; you can redistribute it and/or modify it
+ under the terms of the GNU Lesser General Public License as published by
+ the Free Software Foundation; either version 2.1 of the License, or
+ (at your option) any later version.
+
+ systemd is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public License
+ along with systemd; If not, see <http://www.gnu.org/licenses/>.
+***/
+
+#include <stdlib.h>
+#include <errno.h>
+#include <string.h>
+#include <stdio.h>
+
+#include "util.h"
+#include "list.h"
+
+#include "protocol.h"
+#include "client.h"
+
+struct DHCPClient {
+ DHCPState state;
+ int index;
+ uint8_t *req_opts;
+ int req_opts_size;
+ struct in_addr *last_addr;
+};
+
+static uint8_t default_req_opts[] = {
+ DHCP_OPTION_SUBNET_MASK,
+ DHCP_OPTION_ROUTER,
+ DHCP_OPTION_HOST_NAME,
+ DHCP_OPTION_DOMAIN_NAME,
+ DHCP_OPTION_DOMAIN_NAME_SERVER,
+ DHCP_OPTION_NTP_SERVER,
+};
+
+int dhcp_client_set_request_option(DHCPClient *client, uint8_t option)
+{
+ int i;
+
+ if (!client)
+ return -EINVAL;
+
+ if (client->state != DHCP_STATE_INIT)
+ return -EBUSY;
+
+ switch(option) {
+ case DHCP_OPTION_PAD:
+ case DHCP_OPTION_OVERLOAD:
+ case DHCP_OPTION_MESSAGE_TYPE:
+ case DHCP_OPTION_PARAMETER_REQUEST_LIST:
+ case DHCP_OPTION_END:
+ return -EINVAL;
+
+ default:
+ break;
+ }
+
+ for (i = 0; i < client->req_opts_size; i++)
+ if (client->req_opts[i] == option)
+ return -EEXIST;
+
+ client->req_opts_size++;
+ client->req_opts = realloc(client->req_opts, client->req_opts_size);
+ if (!client->req_opts) {
+ client->req_opts_size = 0;
+ return -ENOBUFS;
+ }
+
+ client->req_opts[client->req_opts_size - 1] = option;
+
+ return 0;
+}
+
+int dhcp_client_set_request_address(DHCPClient *client,
+ struct in_addr *last_addr)
+{
+ if (!client)
+ return -EINVAL;
+
+ if (client->state != DHCP_STATE_INIT)
+ return -EBUSY;
+
+ client->last_addr = malloc(sizeof(struct in_addr));
+ if (!client->last_addr)
+ return -ENOBUFS;
+
+ memcpy(&client->last_addr, last_addr, sizeof(struct in_addr));
+
+ return 0;
+}
+
+int dhcp_client_set_index(DHCPClient *client, int interface_index)
+{
+ if (!client || interface_index < -1)
+ return -EINVAL;
+
+ if (client->state != DHCP_STATE_INIT)
+ return -EBUSY;
+
+ client->index = interface_index;
+
+ return 0;
+}
+
+DHCPClient *dhcp_client_new(void)
+{
+ DHCPClient *client;
+ int i;
+
+ client = new0(DHCPClient, 1);
+ if (!client)
+ return NULL;
+
+ client->state = DHCP_STATE_INIT;
+ client->index = -1;
+
+ client->req_opts_size = sizeof(default_req_opts)
+ / sizeof(default_req_opts[0]);
+
+ client->req_opts = malloc(client->req_opts_size);
+ for (i = 0; i < client->req_opts_size; i++)
+ client->req_opts[i] = default_req_opts[i];
+
+ return client;
+}
diff --git a/src/dhcp/client.h b/src/dhcp/client.h
new file mode 100644
index 0000000..447c03b
--- /dev/null
+++ b/src/dhcp/client.h
@@ -0,0 +1,34 @@
+/*-*- Mode: C; c-basic-offset: 8; indent-tabs-mode: nil -*-*/
+
+#pragma once
+
+/***
+ This file is part of systemd.
+
+ Copyright (C) 2013 Intel Corporation. All rights reserved.
+
+ systemd is free software; you can redistribute it and/or modify it
+ under the terms of the GNU Lesser General Public License as published by
+ the Free Software Foundation; either version 2.1 of the License, or
+ (at your option) any later version.
+
+ systemd is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public License
+ along with systemd; If not, see <http://www.gnu.org/licenses/>.
+***/
+
+#include <netinet/in.h>
+
+struct DHCPClient;
+typedef struct DHCPClient DHCPClient;
+
+int dhcp_client_set_request_option(DHCPClient *client, uint8_t option);
+int dhcp_client_set_request_address(DHCPClient *client,
+ struct in_addr *last_address);
+int dhcp_client_set_index(DHCPClient *client, int interface_index);
+
+DHCPClient *dhcp_client_new(void);
--
1.7.10.4
Patrik Flykt
2013-11-13 21:22:29 UTC
Permalink
Create a new directory to host DHCP components.
---
src/dhcp/protocol.h | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 93 insertions(+)
create mode 100644 src/dhcp/protocol.h

diff --git a/src/dhcp/protocol.h b/src/dhcp/protocol.h
new file mode 100644
index 0000000..3fc4baf
--- /dev/null
+++ b/src/dhcp/protocol.h
@@ -0,0 +1,93 @@
+/*-*- Mode: C; c-basic-offset: 8; indent-tabs-mode: nil -*-*/
+
+#pragma once
+
+/***
+ This file is part of systemd.
+
+ Copyright (C) 2013 Intel Corporation. All rights reserved.
+
+ systemd is free software; you can redistribute it and/or modify it
+ under the terms of the GNU Lesser General Public License as published by
+ the Free Software Foundation; either version 2.1 of the License, or
+ (at your option) any later version.
+
+ systemd is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public License
+ along with systemd; If not, see <http://www.gnu.org/licenses/>.
+***/
+
+#include <netinet/udp.h>
+#include <netinet/ip.h>
+#include <stdint.h>
+
+struct DHCPMessage {
+ uint8_t op;
+ uint8_t htype;
+ uint8_t hlen;
+ uint8_t hops;
+ uint32_t xid;
+ uint16_t secs;
+ uint16_t flags;
+ uint32_t ciaddr;
+ uint32_t yiaddr;
+ uint32_t siaddr;
+ uint32_t giaddr;
+ uint8_t chaddr[16];
+ uint8_t sname[64];
+ uint8_t file[128];
+} __attribute__((packed));
+
+typedef struct DHCPMessage DHCPMessage;
+
+struct DHCPPacket {
+ struct iphdr ip;
+ struct udphdr udp;
+ DHCPMessage dhcp;
+} __attribute__((packed));
+
+typedef struct DHCPPacket DHCPPacket;
+
+enum DHCPState {
+ DHCP_STATE_INIT = 0,
+ DHCP_STATE_SELECTING = 1,
+ DHCP_STATE_INIT_REBOOT = 2,
+ DHCP_STATE_REBOOTING = 3,
+ DHCP_STATE_REQUESTING = 4,
+ DHCP_STATE_BOUND = 5,
+ DHCP_STATE_RENEWING = 6,
+ DHCP_STATE_REBINDING = 7,
+};
+
+typedef enum DHCPState DHCPState;
+
+#define BOOTREQUEST 1
+#define BOOTREPLY 2
+
+#define DHCP_DISCOVER 1
+#define DHCP_OFFER 2
+#define DHCP_REQUEST 3
+#define DHCP_DECLINE 4
+#define DHCP_ACK 5
+#define DHCP_NAK 6
+#define DHCP_RELEASE 7
+
+#define DHCP_OVERLOAD_FILE 1
+#define DHCP_OVERLOAD_SNAME 2
+
+#define DHCP_OPTION_PAD 0
+#define DHCP_OPTION_SUBNET_MASK 1
+#define DHCP_OPTION_ROUTER 3
+#define DHCP_OPTION_DOMAIN_NAME_SERVER 6
+#define DHCP_OPTION_HOST_NAME 12
+#define DHCP_OPTION_DOMAIN_NAME 15
+#define DHCP_OPTION_NTP_SERVER 42
+#define DHCP_OPTION_REQUESTED_IP_ADDRESS 50
+#define DHCP_OPTION_OVERLOAD 52
+#define DHCP_OPTION_MESSAGE_TYPE 53
+#define DHCP_OPTION_PARAMETER_REQUEST_LIST 55
+#define DHCP_OPTION_END 255
--
1.7.10.4
Zbigniew Jędrzejewski-Szmek
2013-11-13 23:33:14 UTC
Permalink
Post by Patrik Flykt
Create a new directory to host DHCP components.
---
src/dhcp/protocol.h | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 93 insertions(+)
create mode 100644 src/dhcp/protocol.h
diff --git a/src/dhcp/protocol.h b/src/dhcp/protocol.h
new file mode 100644
index 0000000..3fc4baf
--- /dev/null
+++ b/src/dhcp/protocol.h
@@ -0,0 +1,93 @@
+/*-*- Mode: C; c-basic-offset: 8; indent-tabs-mode: nil -*-*/
+
+#pragma once
+
+/***
+ This file is part of systemd.
+
+ Copyright (C) 2013 Intel Corporation. All rights reserved.
+
+ systemd is free software; you can redistribute it and/or modify it
+ under the terms of the GNU Lesser General Public License as published by
+ the Free Software Foundation; either version 2.1 of the License, or
+ (at your option) any later version.
+
+ systemd is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public License
+ along with systemd; If not, see <http://www.gnu.org/licenses/>.
+***/
+
+#include <netinet/udp.h>
+#include <netinet/ip.h>
+#include <stdint.h>
+
+struct DHCPMessage {
+ uint8_t op;
+ uint8_t htype;
+ uint8_t hlen;
+ uint8_t hops;
+ uint32_t xid;
+ uint16_t secs;
+ uint16_t flags;
+ uint32_t ciaddr;
+ uint32_t yiaddr;
+ uint32_t siaddr;
+ uint32_t giaddr;
+ uint8_t chaddr[16];
+ uint8_t sname[64];
+ uint8_t file[128];
+} __attribute__((packed));
We have _packed_.
Post by Patrik Flykt
+
+typedef struct DHCPMessage DHCPMessage;
+
+struct DHCPPacket {
+ struct iphdr ip;
+ struct udphdr udp;
+ DHCPMessage dhcp;
+} __attribute__((packed));
+
+typedef struct DHCPPacket DHCPPacket;
+
+enum DHCPState {
+ DHCP_STATE_INIT = 0,
+ DHCP_STATE_SELECTING = 1,
+ DHCP_STATE_INIT_REBOOT = 2,
+ DHCP_STATE_REBOOTING = 3,
+ DHCP_STATE_REQUESTING = 4,
+ DHCP_STATE_BOUND = 5,
+ DHCP_STATE_RENEWING = 6,
+ DHCP_STATE_REBINDING = 7,
+};
+
+typedef enum DHCPState DHCPState;
+
+#define BOOTREQUEST 1
+#define BOOTREPLY 2
+
+#define DHCP_DISCOVER 1
+#define DHCP_OFFER 2
+#define DHCP_REQUEST 3
+#define DHCP_DECLINE 4
+#define DHCP_ACK 5
+#define DHCP_NAK 6
+#define DHCP_RELEASE 7
+
+#define DHCP_OVERLOAD_FILE 1
+#define DHCP_OVERLOAD_SNAME 2
+
+#define DHCP_OPTION_PAD 0
+#define DHCP_OPTION_SUBNET_MASK 1
+#define DHCP_OPTION_ROUTER 3
+#define DHCP_OPTION_DOMAIN_NAME_SERVER 6
+#define DHCP_OPTION_HOST_NAME 12
+#define DHCP_OPTION_DOMAIN_NAME 15
+#define DHCP_OPTION_NTP_SERVER 42
+#define DHCP_OPTION_REQUESTED_IP_ADDRESS 50
+#define DHCP_OPTION_OVERLOAD 52
+#define DHCP_OPTION_MESSAGE_TYPE 53
+#define DHCP_OPTION_PARAMETER_REQUEST_LIST 55
+#define DHCP_OPTION_END 255
Zbyszek
Lennart Poettering
2013-11-13 23:52:32 UTC
Permalink
Post by Patrik Flykt
+struct DHCPMessage {
+ uint8_t op;
+ uint8_t htype;
+ uint8_t hlen;
+ uint8_t hops;
+ uint32_t xid;
+ uint16_t secs;
+ uint16_t flags;
+ uint32_t ciaddr;
+ uint32_t yiaddr;
+ uint32_t siaddr;
+ uint32_t giaddr;
+ uint8_t chaddr[16];
+ uint8_t sname[64];
+ uint8_t file[128];
+} __attribute__((packed));
Use _packed_ please!
Post by Patrik Flykt
+#define BOOTREQUEST 1
+#define BOOTREPLY 2
+
+#define DHCP_DISCOVER 1
+#define DHCP_OFFER 2
+#define DHCP_REQUEST 3
+#define DHCP_DECLINE 4
+#define DHCP_ACK 5
+#define DHCP_NAK 6
+#define DHCP_RELEASE 7
+
+#define DHCP_OVERLOAD_FILE 1
+#define DHCP_OVERLOAD_SNAME 2
+
+#define DHCP_OPTION_PAD 0
+#define DHCP_OPTION_SUBNET_MASK 1
+#define DHCP_OPTION_ROUTER 3
+#define DHCP_OPTION_DOMAIN_NAME_SERVER 6
+#define DHCP_OPTION_HOST_NAME 12
+#define DHCP_OPTION_DOMAIN_NAME 15
+#define DHCP_OPTION_NTP_SERVER 42
+#define DHCP_OPTION_REQUESTED_IP_ADDRESS 50
+#define DHCP_OPTION_OVERLOAD 52
+#define DHCP_OPTION_MESSAGE_TYPE 53
+#define DHCP_OPTION_PARAMETER_REQUEST_LIST 55
+#define DHCP_OPTION_END 255
For defines like these I'd really suggest using anonymous enums. It's a
good thing if the compiler knows these things, not just the
pre-processor...

Lennart
--
Lennart Poettering, Red Hat
Marcel Holtmann
2013-11-13 23:56:20 UTC
Permalink
Hi Lennart,
Post by Lennart Poettering
Post by Patrik Flykt
+#define BOOTREQUEST 1
+#define BOOTREPLY 2
+
+#define DHCP_DISCOVER 1
+#define DHCP_OFFER 2
+#define DHCP_REQUEST 3
+#define DHCP_DECLINE 4
+#define DHCP_ACK 5
+#define DHCP_NAK 6
+#define DHCP_RELEASE 7
+
+#define DHCP_OVERLOAD_FILE 1
+#define DHCP_OVERLOAD_SNAME 2
+
+#define DHCP_OPTION_PAD 0
+#define DHCP_OPTION_SUBNET_MASK 1
+#define DHCP_OPTION_ROUTER 3
+#define DHCP_OPTION_DOMAIN_NAME_SERVER 6
+#define DHCP_OPTION_HOST_NAME 12
+#define DHCP_OPTION_DOMAIN_NAME 15
+#define DHCP_OPTION_NTP_SERVER 42
+#define DHCP_OPTION_REQUESTED_IP_ADDRESS 50
+#define DHCP_OPTION_OVERLOAD 52
+#define DHCP_OPTION_MESSAGE_TYPE 53
+#define DHCP_OPTION_PARAMETER_REQUEST_LIST 55
+#define DHCP_OPTION_END 255
For defines like these I'd really suggest using anonymous enums. It's a
good thing if the compiler knows these things, not just the
pre-processor...
these are wire protocol definitions. What benefit do you gain if the compiler knows them. You always have to handle invalid cases anyway since malicious servers are a reality.

Regards

Marcel
Lennart Poettering
2013-11-14 00:10:07 UTC
Permalink
Post by Marcel Holtmann
Hi Lennart,
Post by Lennart Poettering
Post by Patrik Flykt
+#define BOOTREQUEST 1
+#define BOOTREPLY 2
+
+#define DHCP_DISCOVER 1
+#define DHCP_OFFER 2
+#define DHCP_REQUEST 3
+#define DHCP_DECLINE 4
+#define DHCP_ACK 5
+#define DHCP_NAK 6
+#define DHCP_RELEASE 7
+
+#define DHCP_OVERLOAD_FILE 1
+#define DHCP_OVERLOAD_SNAME 2
+
+#define DHCP_OPTION_PAD 0
+#define DHCP_OPTION_SUBNET_MASK 1
+#define DHCP_OPTION_ROUTER 3
+#define DHCP_OPTION_DOMAIN_NAME_SERVER 6
+#define DHCP_OPTION_HOST_NAME 12
+#define DHCP_OPTION_DOMAIN_NAME 15
+#define DHCP_OPTION_NTP_SERVER 42
+#define DHCP_OPTION_REQUESTED_IP_ADDRESS 50
+#define DHCP_OPTION_OVERLOAD 52
+#define DHCP_OPTION_MESSAGE_TYPE 53
+#define DHCP_OPTION_PARAMETER_REQUEST_LIST 55
+#define DHCP_OPTION_END 255
For defines like these I'd really suggest using anonymous enums. It's a
good thing if the compiler knows these things, not just the
pre-processor...
these are wire protocol definitions. What benefit do you gain if the
compiler knows them. You always have to handle invalid cases anyway
since malicious servers are a reality.
For example, it's nicer to work with gdb if it can resolve them...

Lennart
--
Lennart Poettering, Red Hat
Zbigniew Jędrzejewski-Szmek
2013-11-14 00:28:34 UTC
Permalink
Post by Lennart Poettering
Post by Marcel Holtmann
Hi Lennart,
Post by Lennart Poettering
Post by Patrik Flykt
+#define BOOTREQUEST 1
+#define BOOTREPLY 2
+
+#define DHCP_DISCOVER 1
+#define DHCP_OFFER 2
+#define DHCP_REQUEST 3
+#define DHCP_DECLINE 4
+#define DHCP_ACK 5
+#define DHCP_NAK 6
+#define DHCP_RELEASE 7
+
+#define DHCP_OVERLOAD_FILE 1
+#define DHCP_OVERLOAD_SNAME 2
+
+#define DHCP_OPTION_PAD 0
+#define DHCP_OPTION_SUBNET_MASK 1
+#define DHCP_OPTION_ROUTER 3
+#define DHCP_OPTION_DOMAIN_NAME_SERVER 6
+#define DHCP_OPTION_HOST_NAME 12
+#define DHCP_OPTION_DOMAIN_NAME 15
+#define DHCP_OPTION_NTP_SERVER 42
+#define DHCP_OPTION_REQUESTED_IP_ADDRESS 50
+#define DHCP_OPTION_OVERLOAD 52
+#define DHCP_OPTION_MESSAGE_TYPE 53
+#define DHCP_OPTION_PARAMETER_REQUEST_LIST 55
+#define DHCP_OPTION_END 255
For defines like these I'd really suggest using anonymous enums. It's a
good thing if the compiler knows these things, not just the
pre-processor...
these are wire protocol definitions. What benefit do you gain if the
compiler knows them. You always have to handle invalid cases anyway
since malicious servers are a reality.
For example, it's nicer to work with gdb if it can resolve them...
You'll also get an error if you use an enum value of one kind
in a call requiring an enum of different type.

Zbyszek
Marcel Holtmann
2013-11-14 00:50:41 UTC
Permalink
Hi Zbyszek,
Post by Zbigniew Jędrzejewski-Szmek
Post by Lennart Poettering
Post by Marcel Holtmann
Post by Lennart Poettering
Post by Patrik Flykt
+#define BOOTREQUEST 1
+#define BOOTREPLY 2
+
+#define DHCP_DISCOVER 1
+#define DHCP_OFFER 2
+#define DHCP_REQUEST 3
+#define DHCP_DECLINE 4
+#define DHCP_ACK 5
+#define DHCP_NAK 6
+#define DHCP_RELEASE 7
+
+#define DHCP_OVERLOAD_FILE 1
+#define DHCP_OVERLOAD_SNAME 2
+
+#define DHCP_OPTION_PAD 0
+#define DHCP_OPTION_SUBNET_MASK 1
+#define DHCP_OPTION_ROUTER 3
+#define DHCP_OPTION_DOMAIN_NAME_SERVER 6
+#define DHCP_OPTION_HOST_NAME 12
+#define DHCP_OPTION_DOMAIN_NAME 15
+#define DHCP_OPTION_NTP_SERVER 42
+#define DHCP_OPTION_REQUESTED_IP_ADDRESS 50
+#define DHCP_OPTION_OVERLOAD 52
+#define DHCP_OPTION_MESSAGE_TYPE 53
+#define DHCP_OPTION_PARAMETER_REQUEST_LIST 55
+#define DHCP_OPTION_END 255
For defines like these I'd really suggest using anonymous enums. It's a
good thing if the compiler knows these things, not just the
pre-processor...
these are wire protocol definitions. What benefit do you gain if the
compiler knows them. You always have to handle invalid cases anyway
since malicious servers are a reality.
For example, it's nicer to work with gdb if it can resolve them...
You'll also get an error if you use an enum value of one kind
in a call requiring an enum of different type.
as I said, these are wire protocol definitions. They come in as bits and bytes and not as enums.

Regards

Marcel
Lennart Poettering
2013-11-14 01:00:51 UTC
Permalink
Post by Marcel Holtmann
Post by Zbigniew Jędrzejewski-Szmek
Post by Lennart Poettering
Post by Marcel Holtmann
Post by Lennart Poettering
Post by Patrik Flykt
+#define BOOTREQUEST 1
+#define BOOTREPLY 2
+
+#define DHCP_DISCOVER 1
+#define DHCP_OFFER 2
+#define DHCP_REQUEST 3
+#define DHCP_DECLINE 4
+#define DHCP_ACK 5
+#define DHCP_NAK 6
+#define DHCP_RELEASE 7
+
+#define DHCP_OVERLOAD_FILE 1
+#define DHCP_OVERLOAD_SNAME 2
+
+#define DHCP_OPTION_PAD 0
+#define DHCP_OPTION_SUBNET_MASK 1
+#define DHCP_OPTION_ROUTER 3
+#define DHCP_OPTION_DOMAIN_NAME_SERVER 6
+#define DHCP_OPTION_HOST_NAME 12
+#define DHCP_OPTION_DOMAIN_NAME 15
+#define DHCP_OPTION_NTP_SERVER 42
+#define DHCP_OPTION_REQUESTED_IP_ADDRESS 50
+#define DHCP_OPTION_OVERLOAD 52
+#define DHCP_OPTION_MESSAGE_TYPE 53
+#define DHCP_OPTION_PARAMETER_REQUEST_LIST 55
+#define DHCP_OPTION_END 255
For defines like these I'd really suggest using anonymous enums. It's a
good thing if the compiler knows these things, not just the
pre-processor...
these are wire protocol definitions. What benefit do you gain if the
compiler knows them. You always have to handle invalid cases anyway
since malicious servers are a reality.
For example, it's nicer to work with gdb if it can resolve them...
You'll also get an error if you use an enum value of one kind
in a call requiring an enum of different type.
as I said, these are wire protocol definitions. They come in as bits and bytes and not as enums.
I was suggesting an *anonymous* enum. That doesn't introduce a type or
anything, it just maps name to integers (of any size). That's all. And
it does so on the compiler level, rather than in the pre-processor. This
is useful because gdb knows about the thing then.

Lennart
--
Lennart Poettering, Red Hat
Marcel Holtmann
2013-11-14 01:12:55 UTC
Permalink
Hi Lennart,
Post by Lennart Poettering
Post by Marcel Holtmann
Post by Zbigniew Jędrzejewski-Szmek
Post by Lennart Poettering
Post by Marcel Holtmann
Post by Lennart Poettering
Post by Patrik Flykt
+#define BOOTREQUEST 1
+#define BOOTREPLY 2
+
+#define DHCP_DISCOVER 1
+#define DHCP_OFFER 2
+#define DHCP_REQUEST 3
+#define DHCP_DECLINE 4
+#define DHCP_ACK 5
+#define DHCP_NAK 6
+#define DHCP_RELEASE 7
+
+#define DHCP_OVERLOAD_FILE 1
+#define DHCP_OVERLOAD_SNAME 2
+
+#define DHCP_OPTION_PAD 0
+#define DHCP_OPTION_SUBNET_MASK 1
+#define DHCP_OPTION_ROUTER 3
+#define DHCP_OPTION_DOMAIN_NAME_SERVER 6
+#define DHCP_OPTION_HOST_NAME 12
+#define DHCP_OPTION_DOMAIN_NAME 15
+#define DHCP_OPTION_NTP_SERVER 42
+#define DHCP_OPTION_REQUESTED_IP_ADDRESS 50
+#define DHCP_OPTION_OVERLOAD 52
+#define DHCP_OPTION_MESSAGE_TYPE 53
+#define DHCP_OPTION_PARAMETER_REQUEST_LIST 55
+#define DHCP_OPTION_END 255
For defines like these I'd really suggest using anonymous enums. It's a
good thing if the compiler knows these things, not just the
pre-processor...
these are wire protocol definitions. What benefit do you gain if the
compiler knows them. You always have to handle invalid cases anyway
since malicious servers are a reality.
For example, it's nicer to work with gdb if it can resolve them...
You'll also get an error if you use an enum value of one kind
in a call requiring an enum of different type.
as I said, these are wire protocol definitions. They come in as bits and bytes and not as enums.
I was suggesting an *anonymous* enum. That doesn't introduce a type or
anything, it just maps name to integers (of any size). That's all. And
it does so on the compiler level, rather than in the pre-processor. This
is useful because gdb knows about the thing then.
fair enough on the gdb level. It is only helping you if the enum value is actually used. So for example in assignments and if-clauses or similar. It does not help you values from the wire that you do not process.

I never really cared about gdb here. Either we have builtin debug/trace printing in our protocol code or we use external tools like tcpdump etc. From BlueZ, oFono and ConnMan past experience, the builtin debug/trace printing has been the most useful tool when we had to debug something that is a wire protocol.

Regards

Marcel
Patrik Flykt
2013-11-13 21:22:32 UTC
Permalink
The client test program is the only one to be built so far.
---
Makefile.am | 15 +++++++++++++++
configure.ac | 9 +++++++++
src/dhcp/Makefile | 1 +
3 files changed, 25 insertions(+)
create mode 120000 src/dhcp/Makefile

diff --git a/Makefile.am b/Makefile.am
index 789ca02..cc52f01 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -3739,6 +3739,21 @@ lib_LTLIBRARIES += \
endif

# ------------------------------------------------------------------------------
+if ENABLE_DHCP
+test_dhcp_client_SOURCES = \
+ src/dhcp/protocol.h \
+ src/dhcp/client.h \
+ src/dhcp/client.c \
+ src/dhcp/test-dhcp-client.c
+
+test_dhcp_client_LDADD = \
+ libsystemd-shared.la
+
+tests += \
+ test-dhcp-client
+endif
+
+# ------------------------------------------------------------------------------
if ENABLE_MACHINED
systemd_machined_SOURCES = \
src/machine/machined.c \
diff --git a/configure.ac b/configure.ac
index d0bfcb8..0c28368 100644
--- a/configure.ac
+++ b/configure.ac
@@ -714,6 +714,14 @@ fi
AM_CONDITIONAL(ENABLE_RFKILL, [test "$have_rfkill" = "yes"])

# ------------------------------------------------------------------------------
+have_dhcp=no
+AC_ARG_ENABLE(dhcp, AS_HELP_STRING([--disable-dhcp], [disable dhcp]))
+if test "x$enable_dhcp" != "xno"; then
+ have_dhcp=yes
+fi
+AM_CONDITIONAL(ENABLE_DHCP, [test "$have_dhcp" = "yes"])
+
+# ------------------------------------------------------------------------------
have_logind=no
AC_ARG_ENABLE(logind, AS_HELP_STRING([--disable-logind], [disable login daemon]))
if test "x$enable_logind" != "xno"; then
@@ -1060,6 +1068,7 @@ AC_MSG_RESULT([
randomseed: ${have_randomseed}
backlight: ${have_backlight}
rfkill: ${have_rfkill}
+ dhcp: ${have_dhcp}
logind: ${have_logind}
machined: ${have_machined}
hostnamed: ${have_hostnamed}
diff --git a/src/dhcp/Makefile b/src/dhcp/Makefile
new file mode 120000
index 0000000..d0b0e8e
--- /dev/null
+++ b/src/dhcp/Makefile
@@ -0,0 +1 @@
+../Makefile
\ No newline at end of file
--
1.7.10.4
Tom Gundersen
2013-11-14 00:28:32 UTC
Permalink
On Wed, Nov 13, 2013 at 10:22 PM, Patrik Flykt
Post by Patrik Flykt
The client test program is the only one to be built so far.
---
Makefile.am | 15 +++++++++++++++
configure.ac | 9 +++++++++
src/dhcp/Makefile | 1 +
3 files changed, 25 insertions(+)
create mode 120000 src/dhcp/Makefile
diff --git a/Makefile.am b/Makefile.am
index 789ca02..cc52f01 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -3739,6 +3739,21 @@ lib_LTLIBRARIES += \
endif
# ------------------------------------------------------------------------------
+if ENABLE_DHCP
Maybe we just as well could put this under ENABLE_NETWORKD, to avoid
introducing two config switches? (And I certainly don't want to ifdef
out dhcp support in networkd)
Post by Patrik Flykt
+test_dhcp_client_SOURCES = \
+ src/dhcp/protocol.h \
+ src/dhcp/client.h \
+ src/dhcp/client.c \
+ src/dhcp/test-dhcp-client.c
+
+test_dhcp_client_LDADD = \
+ libsystemd-shared.la
+
+tests += \
+ test-dhcp-client
+endif
+
+# ------------------------------------------------------------------------------
if ENABLE_MACHINED
systemd_machined_SOURCES = \
src/machine/machined.c \
diff --git a/configure.ac b/configure.ac
index d0bfcb8..0c28368 100644
--- a/configure.ac
+++ b/configure.ac
@@ -714,6 +714,14 @@ fi
AM_CONDITIONAL(ENABLE_RFKILL, [test "$have_rfkill" = "yes"])
# ------------------------------------------------------------------------------
+have_dhcp=no
+AC_ARG_ENABLE(dhcp, AS_HELP_STRING([--disable-dhcp], [disable dhcp]))
+if test "x$enable_dhcp" != "xno"; then
+ have_dhcp=yes
+fi
+AM_CONDITIONAL(ENABLE_DHCP, [test "$have_dhcp" = "yes"])
+
+# ------------------------------------------------------------------------------
have_logind=no
AC_ARG_ENABLE(logind, AS_HELP_STRING([--disable-logind], [disable login daemon]))
if test "x$enable_logind" != "xno"; then
@@ -1060,6 +1068,7 @@ AC_MSG_RESULT([
randomseed: ${have_randomseed}
backlight: ${have_backlight}
rfkill: ${have_rfkill}
+ dhcp: ${have_dhcp}
logind: ${have_logind}
machined: ${have_machined}
hostnamed: ${have_hostnamed}
diff --git a/src/dhcp/Makefile b/src/dhcp/Makefile
new file mode 120000
index 0000000..d0b0e8e
--- /dev/null
+++ b/src/dhcp/Makefile
@@ -0,0 +1 @@
+../Makefile
\ No newline at end of file
--
1.7.10.4
_______________________________________________
systemd-devel mailing list
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Tom Gundersen
2013-11-14 00:40:59 UTC
Permalink
Post by Tom Gundersen
On Wed, Nov 13, 2013 at 10:22 PM, Patrik Flykt
Post by Patrik Flykt
The client test program is the only one to be built so far.
---
Makefile.am | 15 +++++++++++++++
configure.ac | 9 +++++++++
src/dhcp/Makefile | 1 +
3 files changed, 25 insertions(+)
create mode 120000 src/dhcp/Makefile
diff --git a/Makefile.am b/Makefile.am
index 789ca02..cc52f01 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -3739,6 +3739,21 @@ lib_LTLIBRARIES += \
endif
# ------------------------------------------------------------------------------
+if ENABLE_DHCP
Maybe we just as well could put this under ENABLE_NETWORKD, to avoid
introducing two config switches? (And I certainly don't want to ifdef
out dhcp support in networkd)
Actually, I take that back. Just compile the dhcp unconditionally, as
it is not even shipped there is no reason not to.

-t
Patrik Flykt
2013-11-13 21:22:35 UTC
Permalink
---
Makefile.am | 10 ++++++++++
src/dhcp/test-dhcp-option.c | 16 +++++++---------
2 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index cc52f01..aeca484 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -3740,6 +3740,15 @@ endif

# ------------------------------------------------------------------------------
if ENABLE_DHCP
+test_dhcp_option_SOURCES = \
+ src/dhcp/protocol.h \
+ src/dhcp/internal.h \
+ src/dhcp/option.c \
+ src/dhcp/test-dhcp-option.c
+
+test_dhcp_option_LDADD = \
+ libsystemd-shared.la
+
test_dhcp_client_SOURCES = \
src/dhcp/protocol.h \
src/dhcp/client.h \
@@ -3750,6 +3759,7 @@ test_dhcp_client_LDADD = \
libsystemd-shared.la

tests += \
+ test-dhcp-option \
test-dhcp-client
endif

diff --git a/src/dhcp/test-dhcp-option.c b/src/dhcp/test-dhcp-option.c
index 25cb17c..9302fd6 100644
--- a/src/dhcp/test-dhcp-option.c
+++ b/src/dhcp/test-dhcp-option.c
@@ -12,23 +12,23 @@

static void test_invalid_buffer_length(void)
{
- struct dhcp_message message;
+ DHCPMessage message;

assert(__dhcp_option_parse(&message, INT32_MIN, NULL, NULL) ==
- -EINVAL);
- assert(__dhcp_option_parse(&message, sizeof(struct dhcp_message),
- NULL, NULL) == -EINVAL);
+ -EINVAL);
+ assert(__dhcp_option_parse(&message, sizeof(DHCPMessage),
+ NULL, NULL) == -EINVAL);
}

static void test_cookie(void)
{
- struct dhcp_message *message;
- uint32_t len = sizeof(struct dhcp_message) + 4;
+ DHCPMessage *message;
+ uint32_t len = sizeof(DHCPMessage) + 4;
uint8_t *opt;

message = malloc0(len);

- opt = (uint8_t *) message + 1;
+ opt = (uint8_t *)(message + 1);
opt[0] = 0xff;

assert(__dhcp_option_parse(message, len, NULL, NULL) == -EINVAL);
@@ -45,8 +45,6 @@ static void test_cookie(void)

int main(int argc, char *argv[])
{
- unsigned int i;
-
test_invalid_buffer_length();
test_cookie();
--
1.7.10.4
Patrik Flykt
2013-11-13 21:22:39 UTC
Permalink
Open a packet socket, create a link level header, send packet and
close socket. Adding it to a separate file makes testing of the
DHCP sending much easier, as the test program can supply any socket
to the DHCP client code.
---
src/dhcp/internal.h | 2 ++
src/dhcp/network.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 65 insertions(+)
create mode 100644 src/dhcp/network.c

diff --git a/src/dhcp/internal.h b/src/dhcp/internal.h
index 65a3bb3..92bd2a1 100644
--- a/src/dhcp/internal.h
+++ b/src/dhcp/internal.h
@@ -25,6 +25,8 @@

#include "protocol.h"

+int __dhcp_network_send_raw_packet(int index, void *packet, int len);
+
int __dhcp_option_append(uint8_t **buf, int *buflen, uint8_t code,
uint8_t optlen, void *optval);

diff --git a/src/dhcp/network.c b/src/dhcp/network.c
new file mode 100644
index 0000000..623251d
--- /dev/null
+++ b/src/dhcp/network.c
@@ -0,0 +1,63 @@
+/***
+ This file is part of systemd.
+
+ Copyright (C) 2013 Intel Corporation. All rights reserved.
+
+ systemd is free software; you can redistribute it and/or modify it
+ under the terms of the GNU Lesser General Public License as published by
+ the Free Software Foundation; either version 2.1 of the License, or
+ (at your option) any later version.
+
+ systemd is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public License
+ along with systemd; If not, see <http://www.gnu.org/licenses/>.
+***/
+
+#include <errno.h>
+#include <sys/types.h>
+#include <sys/socket.h>
+#include <string.h>
+#include <linux/if_packet.h>
+#include <net/ethernet.h>
+#include <stdio.h>
+#include <unistd.h>
+
+#include "internal.h"
+
+int __dhcp_network_send_raw_packet(int index, void *packet, int len)
+{
+ int err, s;
+ struct sockaddr_ll link;
+
+ err = 0;
+
+ s = socket(AF_PACKET, SOCK_DGRAM | SOCK_CLOEXEC, htons(ETH_P_IP));
+ if (s < 0)
+ return -errno;
+
+ memset(&link, 0, sizeof(link));
+
+ link.sll_family = AF_PACKET;
+ link.sll_protocol = htons(ETH_P_IP);
+ link.sll_ifindex = index;
+ link.sll_halen = ETH_ALEN;
+ memset(&link.sll_addr, 0xff, ETH_ALEN);
+
+ if (bind(s, (struct sockaddr *)&link, sizeof(link)) < 0) {
+ err = -errno;
+ close(s);
+ return err;
+ }
+
+ if (sendto(s, packet, len, 0, (struct sockaddr *)&link,
+ sizeof(link)) < 0)
+ err = -errno;
+
+ close(s);
+
+ return err;
+}
--
1.7.10.4
Lennart Poettering
2013-11-14 00:15:36 UTC
Permalink
Post by Patrik Flykt
+int __dhcp_network_send_raw_packet(int index, void *packet, int len);
Shouldn't this be "const void *"?
Post by Patrik Flykt
int __dhcp_option_append(uint8_t **buf, int *buflen, uint8_t code,
buflen should better be size_t, no?
Post by Patrik Flykt
uint8_t optlen, void *optval);
optval again const?
Post by Patrik Flykt
+int __dhcp_network_send_raw_packet(int index, void *packet, int len)
+{
+ int err, s;
+ struct sockaddr_ll link;
+
+ err = 0;
+
+ s = socket(AF_PACKET, SOCK_DGRAM | SOCK_CLOEXEC, htons(ETH_P_IP));
+ if (s < 0)
+ return -errno;
+
+ memset(&link, 0, sizeof(link));
Please initialize the thing when you define it:

struct sockaddr_ll link = {};

It's actually quicker. Ask Zbigniew... ;-)

In cases where this kind of initialization doesn't work, use the zero()
macro for ases like this.
Post by Patrik Flykt
+
+ link.sll_family = AF_PACKET;
+ link.sll_protocol = htons(ETH_P_IP);
+ link.sll_ifindex = index;
+ link.sll_halen = ETH_ALEN;
+ memset(&link.sll_addr, 0xff, ETH_ALEN);
+
+ if (bind(s, (struct sockaddr *)&link, sizeof(link)) < 0) {
Hmm, to never even have to think about aliasing issue we generally try
to use unions for cases like this. In fact in "socket-util.h" you find a
definition of "union sockaddr_union", where you could add
sockaddr_ll. If you then use the union, you can avoid the cast easily by
getting a pointer of the "sa" member of it.
Post by Patrik Flykt
+ err = -errno;
+ close(s);
+ return err;
+ }
+
+ if (sendto(s, packet, len, 0, (struct sockaddr *)&link,
+ sizeof(link)) < 0)
+ err = -errno;
+
+ close(s);
+
+ return err;
+}
Sounds like you want to use _cleanup_close_ here!

Lennart
--
Lennart Poettering, Red Hat
Patrik Flykt
2013-11-13 21:22:37 UTC
Permalink
Add checks for invalid lengths and parameters when using the option
appending function. Add also checks for adding options, see to it
that the resulting array is identical to the array of options added.
---
src/dhcp/test-dhcp-option.c | 71 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 71 insertions(+)

diff --git a/src/dhcp/test-dhcp-option.c b/src/dhcp/test-dhcp-option.c
index a2b79ce..51ed47a 100644
--- a/src/dhcp/test-dhcp-option.c
+++ b/src/dhcp/test-dhcp-option.c
@@ -294,6 +294,75 @@ static void test_options(struct option_desc *desc)
free(message);
}

+static uint8_t result[64] = {
+ 'A', 'B', 'C', 'D',
+};
+
+static uint8_t options[64] = {
+ 'A', 'B', 'C', 'D',
+ 160, 2, 0x11, 0x12,
+ 0,
+ 31, 8, 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37, 0x38,
+ 0,
+ 55, 3, 0x51, 0x52, 0x53,
+ 17, 7, 0x71, 0x72, 0x73, 0x74, 0x75, 0x76, 0x77,
+ 255
+};
+
+static void test_option_set(void)
+{
+ int len, pos, oldlen, i;
+ uint8_t *opt;
+
+ assert(__dhcp_option_append(NULL, NULL, 0, 0, NULL) == -EINVAL);
+
+ len = 0;
+ opt = &result[0];
+ assert(__dhcp_option_append(&opt, NULL, 0, 0, NULL) == -EINVAL);
+ assert(opt == &result[0] && len == 0);
+
+ assert(__dhcp_option_append(&opt, &len, DHCP_OPTION_PAD,
+ 0, NULL) == -ENOBUFS);
+ assert(opt == &result[0] && len == 0);
+
+ opt = &result[4];
+ len = 1;
+ assert(__dhcp_option_append(&opt, &len, DHCP_OPTION_PAD,
+ 0, NULL) >= 0);
+ assert(opt == &result[5] && len == 0);
+
+ pos = 4;
+ len = 60;
+ while (pos < 64 && options[pos] != DHCP_OPTION_END) {
+ opt = &result[pos];
+ oldlen = len;
+
+ assert(__dhcp_option_append(&opt, &len, options[pos],
+ options[pos + 1],
+ &options[pos + 2]) >= 0);
+
+ if (options[pos] == DHCP_OPTION_PAD) {
+ assert(opt == &result[pos + 1]);
+ assert(len == oldlen - 1);
+ pos++;
+ } else {
+ assert(opt == &result[pos + 2 + options[pos + 1]]);
+ assert(len == oldlen - 2 - options[pos + 1]);
+ pos += 2 + options[pos + 1];
+ }
+ }
+
+ for (i = 0; i < pos; i++) {
+ if (verbose)
+ printf("%2d: 0x%02x(0x%02x)\n", i, result[i],
+ options[i]);
+ assert(result[i] == options[i]);
+ }
+
+ if (verbose)
+ printf ("\n");
+}
+
int main(int argc, char *argv[])
{
unsigned int i;
@@ -306,5 +375,7 @@ int main(int argc, char *argv[])
for (i = 0; i < sizeof(option_tests)/sizeof(struct option_desc); i++)
test_options(&option_tests[i]);

+ test_option_set();
+
return 0;
}
--
1.7.10.4
Patrik Flykt
2013-11-13 21:22:33 UTC
Permalink
Add functions to append and parse DHCP options. Not all options
are passed to the callback function, the ones not exposed are
pad, end, message type and overload. If indicated by the overload
option, file and sname fields will be examined for more options.

The option functions are internal to DHCP, add a new header files
for interal function prototypes.
---
src/dhcp/internal.h | 34 ++++++++++
src/dhcp/option.c | 181 +++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 215 insertions(+)
create mode 100644 src/dhcp/internal.h
create mode 100644 src/dhcp/option.c

diff --git a/src/dhcp/internal.h b/src/dhcp/internal.h
new file mode 100644
index 0000000..65a3bb3
--- /dev/null
+++ b/src/dhcp/internal.h
@@ -0,0 +1,34 @@
+/*-*- Mode: C; c-basic-offset: 8; indent-tabs-mode: nil -*-*/
+
+#pragma once
+
+/***
+ This file is part of systemd.
+
+ Copyright (C) 2013 Intel Corporation. All rights reserved.
+
+ systemd is free software; you can redistribute it and/or modify it
+ under the terms of the GNU Lesser General Public License as published by
+ the Free Software Foundation; either version 2.1 of the License, or
+ (at your option) any later version.
+
+ systemd is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public License
+ along with systemd; If not, see <http://www.gnu.org/licenses/>.
+***/
+
+#include <stdint.h>
+
+#include "protocol.h"
+
+int __dhcp_option_append(uint8_t **buf, int *buflen, uint8_t code,
+ uint8_t optlen, void *optval);
+
+typedef int (*dhcp_option_cb_t)(uint8_t code, uint8_t len, uint8_t *option,
+ void *user_data);
+int __dhcp_option_parse(DHCPMessage *message, int32_t len,
+ dhcp_option_cb_t cb, void *user_data);
diff --git a/src/dhcp/option.c b/src/dhcp/option.c
new file mode 100644
index 0000000..f5ca6e7
--- /dev/null
+++ b/src/dhcp/option.c
@@ -0,0 +1,181 @@
+/*-*- Mode: C; c-basic-offset: 8; indent-tabs-mode: nil -*-*/
+
+/***
+ This file is part of systemd.
+
+ Copyright (C) 2013 Intel Corporation. All rights reserved.
+
+ systemd is free software; you can redistribute it and/or modify it
+ under the terms of the GNU Lesser General Public License as published by
+ the Free Software Foundation; either version 2.1 of the License, or
+ (at your option) any later version.
+
+ systemd is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public License
+ along with systemd; If not, see <http://www.gnu.org/licenses/>.
+***/
+
+#include <stdint.h>
+#include <string.h>
+#include <errno.h>
+#include <stdio.h>
+
+#include "internal.h"
+
+int __dhcp_option_append(uint8_t **buf, int *buflen, uint8_t code,
+ uint8_t optlen, void *optval)
+{
+ if (!buf || !buflen)
+ return -EINVAL;
+
+ switch (code) {
+
+ case DHCP_OPTION_PAD:
+ case DHCP_OPTION_END:
+ if (*buflen < 1)
+ return -ENOBUFS;
+
+ (*buf)[0] = code;
+ *buf += 1;
+ *buflen -= 1;
+ break;
+
+ default:
+ if (*buflen < optlen + 2)
+ return -ENOBUFS;
+
+ if (!optval)
+ return -EINVAL;
+
+ (*buf)[0] = code;
+ (*buf)[1] = optlen;
+ memcpy(&(*buf)[2], optval, optlen);
+
+ *buf += optlen + 2;
+ *buflen = *buflen - optlen - 2;
+
+ break;
+ }
+
+ return 0;
+}
+
+static int parse_options(uint8_t *buf, int32_t buflen, int *overload,
+ int *message_type, dhcp_option_cb_t cb,
+ void *user_data)
+{
+ uint8_t *code = buf;
+ uint8_t *len;
+
+ while (buflen > 0) {
+ switch (*code) {
+ case DHCP_OPTION_PAD:
+ buflen -= 1;
+ code++;
+ break;
+
+ case DHCP_OPTION_END:
+ return 0;
+
+ case DHCP_OPTION_MESSAGE_TYPE:
+ buflen -= 3;
+ if (buflen < 0)
+ return -ENOBUFS;
+
+ len = code + 1;
+ if (*len != 1)
+ return -EINVAL;
+
+ if (message_type)
+ *message_type = *(len + 1);
+
+ code = code + 3;
+
+ break;
+
+ case DHCP_OPTION_OVERLOAD:
+ buflen -= 3;
+ if (buflen < 0)
+ return -ENOBUFS;
+
+ len = code + 1;
+ if (*len != 1)
+ return -EINVAL;
+
+ if (overload)
+ *overload = *(len + 1);
+
+ code = code + 3;
+
+ break;
+
+ default:
+ if (buflen < 3)
+ return -ENOBUFS;
+
+ len = code + 1;
+ buflen -= *len + 2;
+
+ if (buflen < 0)
+ return -EINVAL;
+
+ if (cb)
+ cb(*code, *len, len + 1, user_data);
+
+ code = code + *len + 2;
+
+ break;
+ }
+ }
+
+ if (buflen)
+ return -EINVAL;
+
+ return 0;
+}
+
+int __dhcp_option_parse(DHCPMessage *message, int32_t len,
+ dhcp_option_cb_t cb, void *user_data)
+{
+ int overload = 0;
+ int message_type = -ENOMSG;
+ uint8_t *opt = (uint8_t *)(message + 1);
+ int res;
+
+ if (message == NULL || len < 0)
+ return -EINVAL;
+
+ len -= sizeof(DHCPMessage);
+ len -= 4;
+ if (len < 0)
+ return -EINVAL;
+
+ if (opt[0] != 0x63 && opt[1] != 0x82 && opt[2] != 0x53 &&
+ opt[3] != 0x63)
+ return -EINVAL;
+
+ res = parse_options(&opt[4], len, &overload, &message_type,
+ cb, user_data);
+ if (res < 0)
+ return res;
+
+ if (overload & DHCP_OVERLOAD_FILE) {
+ res = parse_options(message->file, sizeof(message->file),
+ NULL, &message_type, cb, user_data);
+ if (res < 0)
+ return res;
+ }
+
+ if (overload & DHCP_OVERLOAD_SNAME) {
+ res = parse_options(message->sname, sizeof(message->sname),
+ NULL, &message_type, cb, user_data);
+ if (res < 0)
+ return res;
+ }
+
+ return message_type;
+}
--
1.7.10.4
Zbigniew Jędrzejewski-Szmek
2013-11-13 23:52:01 UTC
Permalink
Post by Patrik Flykt
Add functions to append and parse DHCP options. Not all options
are passed to the callback function, the ones not exposed are
pad, end, message type and overload. If indicated by the overload
option, file and sname fields will be examined for more options.
The option functions are internal to DHCP, add a new header files
for interal function prototypes.
---
src/dhcp/internal.h | 34 ++++++++++
src/dhcp/option.c | 181 +++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 215 insertions(+)
create mode 100644 src/dhcp/internal.h
create mode 100644 src/dhcp/option.c
diff --git a/src/dhcp/internal.h b/src/dhcp/internal.h
new file mode 100644
index 0000000..65a3bb3
--- /dev/null
+++ b/src/dhcp/internal.h
@@ -0,0 +1,34 @@
+/*-*- Mode: C; c-basic-offset: 8; indent-tabs-mode: nil -*-*/
+
+#pragma once
+
+/***
+ This file is part of systemd.
+
+ Copyright (C) 2013 Intel Corporation. All rights reserved.
+
+ systemd is free software; you can redistribute it and/or modify it
+ under the terms of the GNU Lesser General Public License as published by
+ the Free Software Foundation; either version 2.1 of the License, or
+ (at your option) any later version.
+
+ systemd is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public License
+ along with systemd; If not, see <http://www.gnu.org/licenses/>.
+***/
+
+#include <stdint.h>
+
+#include "protocol.h"
+
+int __dhcp_option_append(uint8_t **buf, int *buflen, uint8_t code,
+ uint8_t optlen, void *optval);
+
+typedef int (*dhcp_option_cb_t)(uint8_t code, uint8_t len, uint8_t *option,
+ void *user_data);
+int __dhcp_option_parse(DHCPMessage *message, int32_t len,
+ dhcp_option_cb_t cb, void *user_data);
Why "__"?
Post by Patrik Flykt
diff --git a/src/dhcp/option.c b/src/dhcp/option.c
new file mode 100644
index 0000000..f5ca6e7
--- /dev/null
+++ b/src/dhcp/option.c
@@ -0,0 +1,181 @@
+/*-*- Mode: C; c-basic-offset: 8; indent-tabs-mode: nil -*-*/
+
+/***
+ This file is part of systemd.
+
+ Copyright (C) 2013 Intel Corporation. All rights reserved.
+
+ systemd is free software; you can redistribute it and/or modify it
+ under the terms of the GNU Lesser General Public License as published by
+ the Free Software Foundation; either version 2.1 of the License, or
+ (at your option) any later version.
+
+ systemd is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public License
+ along with systemd; If not, see <http://www.gnu.org/licenses/>.
+***/
+
+#include <stdint.h>
+#include <string.h>
+#include <errno.h>
+#include <stdio.h>
+
+#include "internal.h"
+
+int __dhcp_option_append(uint8_t **buf, int *buflen, uint8_t code,
+ uint8_t optlen, void *optval)
+{
+ if (!buf || !buflen)
+ return -EINVAL;
+
+ switch (code) {
+
+ if (*buflen < 1)
+ return -ENOBUFS;
+
+ (*buf)[0] = code;
+ *buf += 1;
+ *buflen -= 1;
+ break;
+
+ if (*buflen < optlen + 2)
+ return -ENOBUFS;
+
+ if (!optval)
+ return -EINVAL;
+
+ (*buf)[0] = code;
+ (*buf)[1] = optlen;
+ memcpy(&(*buf)[2], optval, optlen);
+
+ *buf += optlen + 2;
+ *buflen = *buflen - optlen - 2;
+
+ break;
+ }
+
+ return 0;
+}
+
+static int parse_options(uint8_t *buf, int32_t buflen, int *overload,
+ int *message_type, dhcp_option_cb_t cb,
+ void *user_data)
+{
+ uint8_t *code = buf;
+ uint8_t *len;
+
+ while (buflen > 0) {
+ switch (*code) {
+ buflen -= 1;
+ code++;
+ break;
+
+ return 0;
+
+ buflen -= 3;
+ if (buflen < 0)
+ return -ENOBUFS;
+
+ len = code + 1;
+ if (*len != 1)
+ return -EINVAL;
+
+ if (message_type)
+ *message_type = *(len + 1);
+
+ code = code + 3;
+
+ break;
+
+ buflen -= 3;
+ if (buflen < 0)
+ return -ENOBUFS;
+
+ len = code + 1;
+ if (*len != 1)
+ return -EINVAL;
+
+ if (overload)
+ *overload = *(len + 1);
+
+ code = code + 3;
code += 3; ?
Post by Patrik Flykt
+
+ break;
+
+ if (buflen < 3)
+ return -ENOBUFS;
+
+ len = code + 1;
+ buflen -= *len + 2;
+
+ if (buflen < 0)
+ return -EINVAL;
+
+ if (cb)
+ cb(*code, *len, len + 1, user_data);
+
+ code = code + *len + 2;
code += *len + 2; ?
Post by Patrik Flykt
+
+ break;
+ }
+ }
+
+ if (buflen)
+ return -EINVAL;
+
+ return 0;
+}
+
+int __dhcp_option_parse(DHCPMessage *message, int32_t len,
+ dhcp_option_cb_t cb, void *user_data)
+{
+ int overload = 0;
+ int message_type = -ENOMSG;
+ uint8_t *opt = (uint8_t *)(message + 1);
+ int res;
+
+ if (message == NULL || len < 0)
+ return -EINVAL;
+
+ len -= sizeof(DHCPMessage);
+ len -= 4;
+ if (len < 0)
+ return -EINVAL;
+
+ if (opt[0] != 0x63 && opt[1] != 0x82 && opt[2] != 0x53 &&
+ opt[3] != 0x63)
+ return -EINVAL;
+
+ res = parse_options(&opt[4], len, &overload, &message_type,
+ cb, user_data);
+ if (res < 0)
+ return res;
+
+ if (overload & DHCP_OVERLOAD_FILE) {
+ res = parse_options(message->file, sizeof(message->file),
+ NULL, &message_type, cb, user_data);
+ if (res < 0)
+ return res;
+ }
+
+ if (overload & DHCP_OVERLOAD_SNAME) {
+ res = parse_options(message->sname, sizeof(message->sname),
+ NULL, &message_type, cb, user_data);
+ if (res < 0)
+ return res;
+ }
+
+ return message_type;
+}
Zbyszek
Lennart Poettering
2013-11-14 00:06:15 UTC
Permalink
Post by Patrik Flykt
+int __dhcp_option_append(uint8_t **buf, int *buflen, uint8_t code,
+ uint8_t optlen, void *optval);
+
The "__" prefix is actually private property by the C compiler,
according to ANSI C. Please do not define your own symbols in this
namespace. (Also why even?)
Post by Patrik Flykt
+int __dhcp_option_append(uint8_t **buf, int *buflen, uint8_t code,
+ uint8_t optlen, void *optval)
Shouldn't "void *optval" be const?

Lennart
--
Lennart Poettering, Red Hat
Patrik Flykt
2013-11-15 09:22:01 UTC
Permalink
Post by Lennart Poettering
Post by Patrik Flykt
+int __dhcp_option_append(uint8_t **buf, int *buflen, uint8_t code,
+ uint8_t optlen, void *optval);
+
The "__" prefix is actually private property by the C compiler,
according to ANSI C. Please do not define your own symbols in this
namespace. (Also why even?)
Freudian slip, used in other projects related to ConnMan... will fix.

Cheers,

Patrik
Patrik Flykt
2013-11-13 21:22:42 UTC
Permalink
Set a fake MAC address and emulate raw packet sending. When the buffer
containing the Discover message is received, check selected IP and
UDP headers and compute IP header and UDP message checksums. Also
send the DHCP message for option parsing and expect a successful
outcome.
---
Makefile.am | 3 ++
src/dhcp/test-dhcp-client.c | 79 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 82 insertions(+)

diff --git a/Makefile.am b/Makefile.am
index 5c350ab..1c73423 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -3771,6 +3771,9 @@ test_dhcp_option_LDADD = \

test_dhcp_client_SOURCES = \
src/dhcp/protocol.h \
+ src/dhcp/internal.h \
+ src/dhcp/option.h \
+ src/dhcp/option.c \
src/dhcp/client.h \
src/dhcp/client.c \
src/dhcp/test-dhcp-client.c
diff --git a/src/dhcp/test-dhcp-client.c b/src/dhcp/test-dhcp-client.c
index 94f576a..b5dde05 100644
--- a/src/dhcp/test-dhcp-client.c
+++ b/src/dhcp/test-dhcp-client.c
@@ -23,10 +23,16 @@
#include <assert.h>
#include <errno.h>
#include <stdio.h>
+#include <string.h>

#include "protocol.h"
+#include "internal.h"
#include "client.h"

+static struct ether_addr mac_addr = {
+ .ether_addr_octet = {'A', 'B', 'C', '1', '2', '3'}
+};
+
static void test_request_basic(void)
{
DHCPClient *client;
@@ -112,10 +118,83 @@ static void test_checksum(void)
assert(client_checksum(&buf, 20) == *val);
}

+static int check_options(uint8_t code, uint8_t len, uint8_t *option,
+ void *user_data)
+{
+ return 0;
+}
+
+int __dhcp_network_send_raw_packet(int index, void *packet, int len)
+{
+ int size;
+ DHCPPacket *discover;
+ uint16_t ip_check, udp_check;
+ int res;
+
+ assert(index == 42);
+ assert(packet);
+
+ size = sizeof(DHCPPacket) + 4;
+ assert(len > size);
+
+ discover = packet;
+
+ assert(memcmp(discover->dhcp.chaddr,
+ &mac_addr.ether_addr_octet, 6) == 0);
+ assert(discover->ip.ttl == IPDEFTTL);
+ assert(discover->ip.protocol == IPPROTO_UDP);
+ assert(discover->ip.saddr == INADDR_ANY);
+ assert(discover->ip.daddr == INADDR_BROADCAST);
+ assert(discover->udp.source == ntohs(DHCP_PORT_CLIENT));
+ assert(discover->udp.dest == ntohs(DHCP_PORT_SERVER));
+
+ ip_check = discover->ip.check;
+
+ discover->ip.ttl = 0;
+ discover->ip.check = discover->udp.len;
+
+ udp_check = ~client_checksum(&discover->ip.ttl, len - 8);
+ assert(udp_check == 0xffff);
+
+ discover->ip.ttl = IPDEFTTL;
+ discover->ip.check = ip_check;
+
+ ip_check = ~client_checksum(&discover->ip, sizeof(discover->ip));
+ assert(ip_check == 0xffff);
+
+ size = len - sizeof(struct iphdr) - sizeof(struct udphdr);
+
+ res = __dhcp_option_parse(&discover->dhcp, size, check_options, NULL);
+ if (res < 0)
+ return res;
+
+ return 575;
+}
+
+static void test_discover_message(void)
+{
+ DHCPClient *client;
+ int res;
+
+ client = dhcp_client_new();
+ assert(client);
+
+ assert(dhcp_client_set_index(client, 42) >= 0);
+ assert(dhcp_client_set_mac(client, &mac_addr) >= 0);
+
+ assert(dhcp_client_set_request_option(client, 248) >= 0);
+
+ res = dhcp_client_start(client);
+
+ assert(res == 575 || res == -EINPROGRESS);
+}
+
int main(int argc, char *argv[])
{
test_request_basic();
test_checksum();

+ test_discover_message();
+
return 0;
}
--
1.7.10.4
Patrik Flykt
2013-11-13 21:22:44 UTC
Permalink
---
Makefile.am | 10 ++++++++++
1 file changed, 10 insertions(+)

diff --git a/Makefile.am b/Makefile.am
index 1c73423..f079822 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -3760,6 +3760,12 @@ libsystemd_dhcp_la_LDFLAGS = \
libsystemd_dhcp_la_LIBADD = \
libsystemd-shared.la

+dhcp_example_client_SOURCES = \
+ src/dhcp/dhcp-example-client.c
+
+dhcp_example_client_LDADD = \
+ libsystemd-dhcp.la
+
test_dhcp_option_SOURCES = \
src/dhcp/protocol.h \
src/dhcp/internal.h \
@@ -3784,6 +3790,10 @@ test_dhcp_client_LDADD = \
tests += \
test-dhcp-option \
test-dhcp-client
+
+manual_tests += \
+ dhcp-example-client
+
endif

# ------------------------------------------------------------------------------
--
1.7.10.4
Patrik Flykt
2013-11-13 21:22:48 UTC
Permalink
Require a main loop to be set when creating a DHCP client. Set up
a timer to resend DHCP Discover messages and add a 0-2 second
delay to the timeout value. Move to state Selecting after successful
sending of a Discover message.
---
src/dhcp/client.c | 77 ++++++++++++++++++++++++++++++++++++++--
src/dhcp/client.h | 4 ++-
src/dhcp/dhcp-example-client.c | 7 +++-
src/dhcp/test-dhcp-client.c | 19 ++++++----
4 files changed, 96 insertions(+), 11 deletions(-)

diff --git a/src/dhcp/client.c b/src/dhcp/client.c
index 79ac93b..cb3d41b 100644
--- a/src/dhcp/client.c
+++ b/src/dhcp/client.c
@@ -34,12 +34,15 @@

struct DHCPClient {
DHCPState state;
+ sd_event *event;
+ sd_event_source *timeout_resend;
int index;
uint8_t *req_opts;
int req_opts_size;
struct in_addr *last_addr;
struct ether_addr mac_addr;
uint32_t xid;
+ uint64_t start_time;
};

static uint8_t default_req_opts[] = {
@@ -142,6 +145,10 @@ static int client_stop(DHCPClient *client, int error)
client->state == DHCP_STATE_INIT_REBOOT)
return -EALREADY;

+ if (client->timeout_resend)
+ client->timeout_resend =
+ sd_event_source_unref(client->timeout_resend);
+
switch (client->state) {

case DHCP_STATE_INIT:
@@ -294,8 +301,59 @@ error:
return err;
}

+static int client_timeout_resend(sd_event_source *s, uint64_t usec,
+ void *userdata)
+{
+ DHCPClient *client = userdata;
+ uint64_t next_timeout;
+ uint16_t secs;
+ int err = 0;
+
+ switch (client->state) {
+ case DHCP_STATE_INIT:
+ case DHCP_STATE_SELECTING:
+
+ if (!client->start_time)
+ client->start_time = usec;
+
+ secs = (usec - client->start_time) / USEC_PER_SEC;
+
+ next_timeout = usec + 2 * USEC_PER_SEC + (random() & 0x1fffff);
+
+ err = sd_event_add_monotonic(sd_event_get(s), next_timeout,
+ 10 * USEC_PER_MSEC,
+ client_timeout_resend, client,
+ &client->timeout_resend);
+ if (err < 0)
+ goto error;
+
+ if (client_send_discover(client, secs) >= 0)
+ client->state = DHCP_STATE_SELECTING;
+
+ break;
+
+ case DHCP_STATE_INIT_REBOOT:
+ case DHCP_STATE_REBOOTING:
+ case DHCP_STATE_REQUESTING:
+ case DHCP_STATE_BOUND:
+ case DHCP_STATE_RENEWING:
+ case DHCP_STATE_REBINDING:
+
+ break;
+ }
+
+ return 0;
+
+error:
+ client_stop(client, err);
+
+ return 0;
+}
+
int dhcp_client_start(DHCPClient *client)
{
+ int err;
+
if (!client || client->index == -1)
return -EINVAL;

@@ -305,7 +363,18 @@ int dhcp_client_start(DHCPClient *client)

client->xid = random_u();

- return client_send_discover(client, 0);
+ err = sd_event_add_monotonic(client->event, now(CLOCK_MONOTONIC), 0,
+ client_timeout_resend, client,
+ &client->timeout_resend);
+ if (err < 0)
+ goto error;
+
+ return 0;
+
+error:
+ client_stop(client, err);
+
+ return err;
}

int dhcp_client_stop(DHCPClient *client)
@@ -313,15 +382,19 @@ int dhcp_client_stop(DHCPClient *client)
return client_stop(client, 0);
}

-DHCPClient *dhcp_client_new(void)
+DHCPClient *dhcp_client_new(sd_event *event)
{
DHCPClient *client;
int i;

+ if (!event)
+ return NULL;
+
client = new0(DHCPClient, 1);
if (!client)
return NULL;

+ client->event = event;
client->state = DHCP_STATE_INIT;
client->index = -1;

diff --git a/src/dhcp/client.h b/src/dhcp/client.h
index 71fdbe3..568e70a 100644
--- a/src/dhcp/client.h
+++ b/src/dhcp/client.h
@@ -24,6 +24,8 @@
#include <netinet/in.h>
#include <net/ethernet.h>

+#include "sd-event.h"
+
struct DHCPClient;
typedef struct DHCPClient DHCPClient;

@@ -35,4 +37,4 @@ int dhcp_client_set_mac(DHCPClient *client, struct ether_addr *addr);

int dhcp_client_start(DHCPClient *client);
int dhcp_client_stop(DHCPClient *client);
-DHCPClient *dhcp_client_new(void);
+DHCPClient *dhcp_client_new(sd_event *event);
diff --git a/src/dhcp/dhcp-example-client.c b/src/dhcp/dhcp-example-client.c
index b7f1a5f..afb7102 100644
--- a/src/dhcp/dhcp-example-client.c
+++ b/src/dhcp/dhcp-example-client.c
@@ -84,6 +84,7 @@ int main(int argc, char **argv)
DHCPClient *client;
int err, index;
struct ether_addr mac;
+ sd_event *event;

if (argc != 2) {
printf("Usage: %s <interface>\n", argv[0]);
@@ -98,7 +99,10 @@ int main(int argc, char **argv)
if (err < 0)
return 2;

- client = dhcp_client_new();
+ if (sd_event_new(&event) < 0)
+ return 2;
+
+ client = dhcp_client_new(event);
if (!client)
return 3;

@@ -107,6 +111,7 @@ int main(int argc, char **argv)
dhcp_client_set_mac(client, &mac);

dhcp_client_start(client);
+ sd_event_loop(event);

return 0;
}
diff --git a/src/dhcp/test-dhcp-client.c b/src/dhcp/test-dhcp-client.c
index b5dde05..9cad04b 100644
--- a/src/dhcp/test-dhcp-client.c
+++ b/src/dhcp/test-dhcp-client.c
@@ -33,11 +33,11 @@ static struct ether_addr mac_addr = {
.ether_addr_octet = {'A', 'B', 'C', '1', '2', '3'}
};

-static void test_request_basic(void)
+static void test_request_basic(sd_event *e)
{
DHCPClient *client;

- client = dhcp_client_new();
+ client = dhcp_client_new(e);

assert(client);

@@ -171,12 +171,12 @@ int __dhcp_network_send_raw_packet(int index, void *packet, int len)
return 575;
}

-static void test_discover_message(void)
+static void test_discover_message(sd_event *e)
{
DHCPClient *client;
int res;

- client = dhcp_client_new();
+ client = dhcp_client_new(e);
assert(client);

assert(dhcp_client_set_index(client, 42) >= 0);
@@ -186,15 +186,20 @@ static void test_discover_message(void)

res = dhcp_client_start(client);

- assert(res == 575 || res == -EINPROGRESS);
+ assert(res == 575 || res == 0);
}

int main(int argc, char *argv[])
{
- test_request_basic();
+ sd_event *e;
+
+ assert(sd_event_new(&e) >= 0);
+
+ test_request_basic(e);
test_checksum();

- test_discover_message();
+ test_discover_message(e);
+ sd_event_run(e, (uint64_t) -1);

return 0;
}
--
1.7.10.4
Lennart Poettering
2013-11-14 00:30:22 UTC
Permalink
Post by Patrik Flykt
int index;
uint8_t *req_opts;
int req_opts_size;
struct in_addr *last_addr;
struct ether_addr mac_addr;
uint32_t xid;
+ uint64_t start_time;
Internally we prefer "usec_t" for times, since that indicates the unit
used here. usec_t is actually identical to uint64_t, but it's more
descriptive, hence preferred. Only in the public APIs we restraint
ourselves a bit and expose this as uint64_t, to not pollute the
namespace.
Post by Patrik Flykt
+ if (!client->start_time)
+ client->start_time = usec;
+
+ secs = (usec - client->start_time) / USEC_PER_SEC;
+
+ next_timeout = usec + 2 * USEC_PER_SEC + (random() & 0x1fffff);
+
+ err = sd_event_add_monotonic(sd_event_get(s), next_timeout,
+ 10 * USEC_PER_MSEC,
+ client_timeout_resend, client,
+
&client->timeout_resend);
if you don't have a very good reason to specify the accuracy as 10ms,
I'd always recommend to pass 0 instead, which results in the default
accuracy of 250ms (I wouldn't be too surprised if 250ms is too
inaccurate for this usecase, so your code might be fine, just wanted to
take the opportuntine to point this out...
Post by Patrik Flykt
- return client_send_discover(client, 0);
+ err = sd_event_add_monotonic(client->event, now(CLOCK_MONOTONIC), 0,
+ client_timeout_resend, client,
+ &client->timeout_resend);
Hmm, so this would immediately trigger since "now" is already passed, by
the time you read it... Note that "0" can be used as time here too, to
indicate that you want to be executed immediately, i.e. it indicates in
this context the ealiest sensible time.

Lennart
--
Lennart Poettering, Red Hat
Patrik Flykt
2013-11-15 09:29:49 UTC
Permalink
Hi,
Post by Patrik Flykt
Post by Patrik Flykt
+ err = sd_event_add_monotonic(sd_event_get(s),
next_timeout,
Post by Patrik Flykt
+ 10 * USEC_PER_MSEC,
+ client_timeout_resend,
client,
Post by Patrik Flykt
+
&client->timeout_resend);
if you don't have a very good reason to specify the accuracy as 10ms,
I'd always recommend to pass 0 instead, which results in the default
accuracy of 250ms (I wouldn't be too surprised if 250ms is too
inaccurate for this usecase, so your code might be fine, just wanted to
take the opportuntine to point this out...
I tried to figure out some reasonable accuracy for sending the DHCP
messages so that hordes of clients would not trigger all at the same
time. The default 250ms seems to be a too coarse interval for this, 10ms
looked decently low enough to spread out the requests without being
overly aggressive. At some point there will be real numbers from real
use cases and the accuracy should be adjusted accordingly.
Post by Patrik Flykt
Post by Patrik Flykt
- return client_send_discover(client, 0);
+ err = sd_event_add_monotonic(client->event,
now(CLOCK_MONOTONIC), 0,
Post by Patrik Flykt
+ client_timeout_resend, client,
+ &client->timeout_resend);
Hmm, so this would immediately trigger since "now" is already passed, by
the time you read it... Note that "0" can be used as time here too, to
indicate that you want to be executed immediately, i.e. it indicates in
this context the ealiest sensible time.
Bummer. I was thinking that the new event would be run immediately after
the current one had returned to the main loop (or actually started in
this case). The general idea was to do the resending in one place only
and keep the new event handling clear from possible interference with
the currently running one, if needed. 'now(CLOCK_MONOTONIC) + 1' would
be enough in those cases, I'd guess. But in here it'd be easier to just
call the function directly. I'll check the other related parts also.

Thanks for all the comments in getting me on track with systemd
internals. I'll update the patch set according to your and zbvszek's
comments.


Cheers,

Patrik
Lennart Poettering
2013-11-15 12:36:44 UTC
Permalink
Post by Patrik Flykt
Post by Patrik Flykt
Post by Patrik Flykt
+ err = sd_event_add_monotonic(sd_event_get(s),
next_timeout,
Post by Patrik Flykt
+ 10 * USEC_PER_MSEC,
+ client_timeout_resend,
client,
Post by Patrik Flykt
+
&client->timeout_resend);
if you don't have a very good reason to specify the accuracy as 10ms,
I'd always recommend to pass 0 instead, which results in the default
accuracy of 250ms (I wouldn't be too surprised if 250ms is too
inaccurate for this usecase, so your code might be fine, just wanted to
take the opportuntine to point this out...
I tried to figure out some reasonable accuracy for sending the DHCP
messages so that hordes of clients would not trigger all at the same
time. The default 250ms seems to be a too coarse interval for this, 10ms
looked decently low enough to spread out the requests without being
overly aggressive. At some point there will be real numbers from real
use cases and the accuracy should be adjusted accordingly.
Just to take the opportunity to talk about this awesome feature of
sd-event: what the accuracy controls is a time range in which the event
will fire, that starts with the specified timeout time and lasts until
that timeout time plus the accuracy. Within that range sd-event will try
to find a good time to wake up in order to optimize power
consumption. For that it tries to move the wakeup across all processes
to the same point in time within each second, and if that point in time
does not lie within the desired range, then it will try to move it to
the same place within each 250ms. If the range doesn't allow that
either, then it will put the wakeup at the end of the desired range. The
point within the same second/the same 250ms is calculated from
/etc/machine-id which is randomized but constant for each system and
hopefully unique in the network, thus avoiding traffic floods. Putting
this together we should minimize wakeups, and if we do wakeup then we
should do work across all processes, but at different times on different
computers.
Post by Patrik Flykt
Post by Patrik Flykt
Post by Patrik Flykt
- return client_send_discover(client, 0);
+ err = sd_event_add_monotonic(client->event,
now(CLOCK_MONOTONIC), 0,
Post by Patrik Flykt
+ client_timeout_resend, client,
+ &client->timeout_resend);
Hmm, so this would immediately trigger since "now" is already passed, by
the time you read it... Note that "0" can be used as time here too, to
indicate that you want to be executed immediately, i.e. it indicates in
this context the ealiest sensible time.
Bummer. I was thinking that the new event would be run immediately after
the current one had returned to the main loop (or actually started in
this case).
Yes, that is what I meant with "immediately". sd-event is not recursive,
it will never dispatch an event from another event. Basically, each time
an event has been dispatched we just determine the next one to dispatch
by looking for the "oldest" one queued. If you specify 0 as a time the
event source is necessarily as "old" as an event source can be.
Post by Patrik Flykt
The general idea was to do the resending in one place only
and keep the new event handling clear from possible interference with
the currently running one, if needed. 'now(CLOCK_MONOTONIC) + 1' would
be enough in those cases, I'd guess. But in here it'd be easier to just
call the function directly. I'll check the other related parts also.
Passing 0 for this is totally OK. My comment was about using 0 instead
of now(), since the latter is unnecessary and calls a syscall for no
point if all you need is to schedule another wakeup.

Lennart
--
Lennart Poettering, Red Hat
Patrik Flykt
2013-11-13 21:22:43 UTC
Permalink
The test program runs the DHCP protocol using libsystemd-dhcp.
---
src/dhcp/dhcp-example-client.c | 112 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 112 insertions(+)
create mode 100644 src/dhcp/dhcp-example-client.c

diff --git a/src/dhcp/dhcp-example-client.c b/src/dhcp/dhcp-example-client.c
new file mode 100644
index 0000000..b7f1a5f
--- /dev/null
+++ b/src/dhcp/dhcp-example-client.c
@@ -0,0 +1,112 @@
+/***
+ This file is part of systemd.
+
+ Copyright (C) 2013 Intel Corporation. All rights reserved.
+
+ systemd is free software; you can redistribute it and/or modify it
+ under the terms of the GNU Lesser General Public License as published by
+ the Free Software Foundation; either version 2.1 of the License, or
+ (at your option) any later version.
+
+ systemd is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public License
+ along with systemd; If not, see <http://www.gnu.org/licenses/>.
+***/
+
+#include <stdio.h>
+#include <sys/ioctl.h>
+#include <net/if.h>
+#include <string.h>
+#include <unistd.h>
+#include <net/ethernet.h>
+#include <errno.h>
+
+#include "client.h"
+
+static int get_mac(int index, struct ether_addr *mac)
+{
+ struct ifreq ifr;
+ int s, err;
+
+ if (index < 0 || !mac)
+ return -EINVAL;
+
+ s = socket(PF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0);
+ if (s < 0)
+ return -EIO;
+
+ memset(&ifr, 0, sizeof(ifr));
+ ifr.ifr_ifindex = index;
+
+ if (ioctl(s, SIOCGIFNAME, &ifr) < 0) {
+ err = -errno;
+ close(s);
+ return err;
+ }
+
+ if (ioctl(s, SIOCGIFHWADDR, &ifr) < 0) {
+ err = -errno;
+ close(s);
+ return err;
+ }
+
+ memcpy(&mac->ether_addr_octet[0], &ifr.ifr_hwaddr.sa_data, ETH_ALEN);
+
+ return 0;
+}
+
+static int get_index(char *ifname)
+{
+ struct ifreq ifr;
+ int s;
+
+ s = socket(PF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0);
+ if (s < 0)
+ return -EIO;
+
+ memset(&ifr, 0, sizeof(ifr));
+ strncpy(ifr.ifr_name, ifname, IFNAMSIZ);
+
+ if (ioctl(s, SIOCGIFINDEX, &ifr) < 0) {
+ close(s);
+ return -EIO;
+ }
+
+ return ifr.ifr_ifindex;
+}
+
+int main(int argc, char **argv)
+{
+ DHCPClient *client;
+ int err, index;
+ struct ether_addr mac;
+
+ if (argc != 2) {
+ printf("Usage: %s <interface>\n", argv[0]);
+ return 1;
+ }
+
+ index = get_index(argv[1]);
+ if (index < 0)
+ return 2;
+
+ err = get_mac(index, &mac);
+ if (err < 0)
+ return 2;
+
+ client = dhcp_client_new();
+ if (!client)
+ return 3;
+
+ printf("Interface %s index %d\n", argv[1], index);
+ dhcp_client_set_index(client, index);
+ dhcp_client_set_mac(client, &mac);
+
+ dhcp_client_start(client);
+
+ return 0;
+}
--
1.7.10.4
Lennart Poettering
2013-11-14 00:22:20 UTC
Permalink
Post by Patrik Flykt
+static int get_mac(int index, struct ether_addr *mac)
+{
+ struct ifreq ifr;
+ int s, err;
+
+ if (index < 0 || !mac)
+ return -EINVAL;
+
+ s = socket(PF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0);
+ if (s < 0)
+ return -EIO;
+
+ memset(&ifr, 0, sizeof(ifr));
Please initialize when declaring with = {} instead.
Post by Patrik Flykt
+ ifr.ifr_ifindex = index;
+
+ if (ioctl(s, SIOCGIFNAME, &ifr) < 0) {
+ err = -errno;
+ close(s);
+ return err;
+ }
+
+ if (ioctl(s, SIOCGIFHWADDR, &ifr) < 0) {
+ err = -errno;
+ close(s);
+ return err;
+ }
+
+ memcpy(&mac->ether_addr_octet[0], &ifr.ifr_hwaddr.sa_data, ETH_ALEN);
+
+ return 0;
You forget to close the socket on success.. But this looks like
something where you should use Tom's rtnl stuff, no? I am pretty sure we
should avoid the ioctls wherever netlink equivalents exist.

Also, _cleanup_close_ is your friend!
Post by Patrik Flykt
+}
+
+static int get_index(char *ifname)
const char *ifname, no?
Post by Patrik Flykt
+{
+ struct ifreq ifr;
+ int s;
+
+ s = socket(PF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0);
+ if (s < 0)
+ return -EIO;
+
+ memset(&ifr, 0, sizeof(ifr));
+ strncpy(ifr.ifr_name, ifname, IFNAMSIZ);
+
+ if (ioctl(s, SIOCGIFINDEX, &ifr) < 0) {
+ close(s);
+ return -EIO;
+ }
+
+ return ifr.ifr_ifindex;
+}
You forgot to close the fd here, too. _cleanup_close_ is your friend
again... Also initialization.

Also, rtnl sounds the better option. And if not, then this call appears
to be totally identical to glibc's ifname_to_index(), no? Better use
that then...

Lennart
--
Lennart Poettering, Red Hat
Patrik Flykt
2013-11-15 09:23:55 UTC
Permalink
Post by Lennart Poettering
Also, rtnl sounds the better option. And if not, then this call appears
to be totally identical to glibc's ifname_to_index(), no? Better use
that then...
This is only part of the example DHCP test client. It's usefulness is
restricted to get DHCP messages being sent on the wire for testing
purposes, but for that purpose it's decently nice. I'm not really sure
whether the test client actually belongs here in the code in the first
place. If I go the rtnl way, someone might get the impression that this
can be
expanded to something useful which is not the intention as
systemd-network already exists.

Cheers,

Patrik
Patrik Flykt
2013-11-13 21:22:51 UTC
Permalink
Add maximum message size option to keep some DHCP server implementations
from sending too big messages. See ConnMan commit
0c5c862749c05193cf4c513628328c6db02b5222.
---
src/dhcp/client.c | 10 ++++++++++
src/dhcp/protocol.h | 1 +
2 files changed, 11 insertions(+)

diff --git a/src/dhcp/client.c b/src/dhcp/client.c
index 4b316dc..45c62f3 100644
--- a/src/dhcp/client.c
+++ b/src/dhcp/client.c
@@ -206,6 +206,7 @@ static int client_packet_init(DHCPClient *client, uint8_t type,
uint8_t **opt, int *optlen)
{
int err;
+ uint16_t max_size;

*opt = (uint8_t *)(message + 1);

@@ -245,6 +246,15 @@ static int client_packet_init(DHCPClient *client, uint8_t type,
client->req_opts);
if (err < 0)
return err;
+
+ max_size = htons(DHCP_IP_UDP_SIZE + DHCP_MESSAGE_SIZE +
+ DHCP_CLIENT_MIN_OPTIONS_SIZE);
+
+ err = __dhcp_option_append(opt, optlen,
+ DHCP_OPTION_MAXIMUM_MESSAGE_SIZE,
+ 2, &max_size);
+ if (err < 0)
+ return err;
}

return 0;
diff --git a/src/dhcp/protocol.h b/src/dhcp/protocol.h
index 014a6f8..6503931 100644
--- a/src/dhcp/protocol.h
+++ b/src/dhcp/protocol.h
@@ -99,5 +99,6 @@ typedef enum DHCPState DHCPState;
#define DHCP_OPTION_MESSAGE_TYPE 53
#define DHCP_OPTION_SERVER_IDENTIFIER 54
#define DHCP_OPTION_PARAMETER_REQUEST_LIST 55
+#define DHCP_OPTION_MAXIMUM_MESSAGE_SIZE 57
#define DHCP_OPTION_CLIENT_IDENTIFIER 61
#define DHCP_OPTION_END 255
--
1.7.10.4
Patrik Flykt
2013-11-13 21:22:56 UTC
Permalink
Print out the event received and possibly also IP related data.
---
src/dhcp/dhcp-example-client.c | 53 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 53 insertions(+)

diff --git a/src/dhcp/dhcp-example-client.c b/src/dhcp/dhcp-example-client.c
index afb7102..3635bd8 100644
--- a/src/dhcp/dhcp-example-client.c
+++ b/src/dhcp/dhcp-example-client.c
@@ -24,6 +24,9 @@
#include <unistd.h>
#include <net/ethernet.h>
#include <errno.h>
+#include <sys/socket.h>
+#include <netinet/in.h>
+#include <arpa/inet.h>

#include "client.h"

@@ -79,6 +82,52 @@ static int get_index(char *ifname)
return ifr.ifr_ifindex;
}

+static const char *print_event(int event)
+{
+ if (event < 0)
+ return strerror(-event);
+
+ switch (event) {
+ case DHCP_EVENT_STOP:
+ return "Stopped";
+ case DHCP_EVENT_NAK:
+ return "DHCP NAK";
+ case DHCP_EVENT_IP_ACQUIRE:
+ return "DHCP address acquired";
+ case DHCP_EVENT_IP_CHANGE:
+ return "DHCP address changed";
+ case DHCP_EVENT_EXPIRED:
+ return "DHCP lease expired";
+ default:
+ break;
+ }
+
+ return "unknown";
+}
+
+static void print_state(DHCPClient *client, int event, void *userdata)
+{
+ sd_event *e = userdata;
+ struct in_addr addr;
+
+ if (event < 0) {
+ printf("Error %d %s\n", event, print_event(event));
+ sd_event_unref(e);
+ return;
+ }
+
+ printf("Event %s\n", print_event(event));
+
+ if (dhcp_client_get_address(client, &addr) >= 0)
+ printf("Address %s\n", inet_ntoa(addr));
+
+ if (dhcp_client_get_netmask(client, &addr) >= 0)
+ printf("Netmask %s\n", inet_ntoa(addr));
+
+ if (dhcp_client_get_router(client, &addr) >= 0)
+ printf("Default router %s\n", inet_ntoa(addr));
+}
+
int main(int argc, char **argv)
{
DHCPClient *client;
@@ -109,9 +158,13 @@ int main(int argc, char **argv)
printf("Interface %s index %d\n", argv[1], index);
dhcp_client_set_index(client, index);
dhcp_client_set_mac(client, &mac);
+ dhcp_client_set_callback(client, print_state, &event);

dhcp_client_start(client);
sd_event_loop(event);

+ dhcp_client_free(client);
+ printf("Exit\n");
+
return 0;
}
--
1.7.10.4
Zbigniew Jędrzejewski-Szmek
2013-11-14 01:52:45 UTC
Permalink
Post by Patrik Flykt
Print out the event received and possibly also IP related data.
---
src/dhcp/dhcp-example-client.c | 53 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 53 insertions(+)
diff --git a/src/dhcp/dhcp-example-client.c b/src/dhcp/dhcp-example-client.c
index afb7102..3635bd8 100644
--- a/src/dhcp/dhcp-example-client.c
+++ b/src/dhcp/dhcp-example-client.c
@@ -24,6 +24,9 @@
#include <unistd.h>
#include <net/ethernet.h>
#include <errno.h>
+#include <sys/socket.h>
+#include <netinet/in.h>
+#include <arpa/inet.h>
#include "client.h"
@@ -79,6 +82,52 @@ static int get_index(char *ifname)
return ifr.ifr_ifindex;
}
+static const char *print_event(int event)
Can this be called event_to_string?
Post by Patrik Flykt
+{
+ if (event < 0)
+ return strerror(-event);
+
+ switch (event) {
+ return "Stopped";
+ return "DHCP NAK";
+ return "DHCP address acquired";
+ return "DHCP address changed";
+ return "DHCP lease expired";
+ break;
+ }
+
+ return "unknown";
+}
+
+static void print_state(DHCPClient *client, int event, void *userdata)
+{
+ sd_event *e = userdata;
+ struct in_addr addr;
+
+ if (event < 0) {
+ printf("Error %d %s\n", event, print_event(event));
+ sd_event_unref(e);
+ return;
+ }
So e is unrefed only sometimes... This seems confusing.
Post by Patrik Flykt
+
+ printf("Event %s\n", print_event(event));
+
+ if (dhcp_client_get_address(client, &addr) >= 0)
+ printf("Address %s\n", inet_ntoa(addr));
+
+ if (dhcp_client_get_netmask(client, &addr) >= 0)
+ printf("Netmask %s\n", inet_ntoa(addr));
+
+ if (dhcp_client_get_router(client, &addr) >= 0)
+ printf("Default router %s\n", inet_ntoa(addr));
+}
+
int main(int argc, char **argv)
{
DHCPClient *client;
@@ -109,9 +158,13 @@ int main(int argc, char **argv)
printf("Interface %s index %d\n", argv[1], index);
dhcp_client_set_index(client, index);
dhcp_client_set_mac(client, &mac);
+ dhcp_client_set_callback(client, print_state, &event);
dhcp_client_start(client);
sd_event_loop(event);
+ dhcp_client_free(client);
+ printf("Exit\n");
+
return 0;
}
Hm, since this is just example code, then probably it doesn't matter that
much :)

Zbyszek
Patrik Flykt
2013-11-13 21:22:31 UTC
Permalink
---
src/dhcp/test-dhcp-client.c | 81 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 81 insertions(+)
create mode 100644 src/dhcp/test-dhcp-client.c

diff --git a/src/dhcp/test-dhcp-client.c b/src/dhcp/test-dhcp-client.c
new file mode 100644
index 0000000..26857b6
--- /dev/null
+++ b/src/dhcp/test-dhcp-client.c
@@ -0,0 +1,81 @@
+/*-*- Mode: C; c-basic-offset: 8; indent-tabs-mode: nil -*-*/
+
+/***
+ This file is part of systemd.
+
+ Copyright (C) 2013 Intel Corporation. All rights reserved.
+
+ systemd is free software; you can redistribute it and/or modify it
+ under the terms of the GNU Lesser General Public License as published by
+ the Free Software Foundation; either version 2.1 of the License, or
+ (at your option) any later version.
+
+ systemd is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public License
+ along with systemd; If not, see <http://www.gnu.org/licenses/>.
+***/
+
+#include <stdlib.h>
+#include <assert.h>
+#include <errno.h>
+
+#include "protocol.h"
+#include "client.h"
+
+static void test_request_basic(void)
+{
+ DHCPClient *client;
+
+ client = dhcp_client_new();
+
+ assert(client);
+
+ assert(dhcp_client_set_request_option(NULL, 0) == -EINVAL);
+ assert(dhcp_client_set_request_address(NULL, NULL) == -EINVAL);
+ assert(dhcp_client_set_index(NULL, 0) == -EINVAL);
+
+ assert(dhcp_client_set_index(client, 15) == 0);
+ assert(dhcp_client_set_index(client, -42) == -EINVAL);
+ assert(dhcp_client_set_index(client, -1) == 0);
+
+ assert(dhcp_client_set_request_option(client,
+ DHCP_OPTION_SUBNET_MASK) == -EEXIST);
+ assert(dhcp_client_set_request_option(client,
+ DHCP_OPTION_ROUTER) == -EEXIST);
+ assert(dhcp_client_set_request_option(client,
+ DHCP_OPTION_HOST_NAME) == -EEXIST);
+ assert(dhcp_client_set_request_option(client,
+ DHCP_OPTION_DOMAIN_NAME) == -EEXIST);
+ assert(dhcp_client_set_request_option(client,
+ DHCP_OPTION_DOMAIN_NAME_SERVER)
+ == -EEXIST);
+ assert(dhcp_client_set_request_option(client,
+ DHCP_OPTION_NTP_SERVER) == -EEXIST);
+
+ assert(dhcp_client_set_request_option(client,
+ DHCP_OPTION_PAD) == -EINVAL);
+ assert(dhcp_client_set_request_option(client,
+ DHCP_OPTION_END) == -EINVAL);
+ assert(dhcp_client_set_request_option(client,
+ DHCP_OPTION_MESSAGE_TYPE) == -EINVAL);
+ assert(dhcp_client_set_request_option(client,
+ DHCP_OPTION_OVERLOAD) == -EINVAL);
+ assert(dhcp_client_set_request_option(client,
+ DHCP_OPTION_PARAMETER_REQUEST_LIST)
+ == -EINVAL);
+
+ assert(dhcp_client_set_request_option(client, 33) == 0);
+ assert(dhcp_client_set_request_option(client, 33) == -EEXIST);
+ assert(dhcp_client_set_request_option(client, 44) == 0);
+}
+
+int main(int argc, char *argv[])
+{
+ test_request_basic();
+
+ return 0;
+}
--
1.7.10.4
Patrik Flykt
2013-11-13 21:22:47 UTC
Permalink
---
Makefile.am | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index f079822..433383d 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -3758,7 +3758,8 @@ libsystemd_dhcp_la_LDFLAGS = \
$(AM_LDFLAGS)

libsystemd_dhcp_la_LIBADD = \
- libsystemd-shared.la
+ libsystemd-shared.la \
+ libsystemd-bus.la

dhcp_example_client_SOURCES = \
src/dhcp/dhcp-example-client.c
@@ -3785,7 +3786,8 @@ test_dhcp_client_SOURCES = \
src/dhcp/test-dhcp-client.c

test_dhcp_client_LDADD = \
- libsystemd-shared.la
+ libsystemd-shared.la \
+ libsystemd-bus.la

tests += \
test-dhcp-option \
--
1.7.10.4
Patrik Flykt
2013-11-13 21:22:40 UTC
Permalink
On starting the client, use the supplied interface mac address and create
a transaction id. Puzzle together an IP/UDP/DHCP Discover message, compute
checksums and send it out as a raw packet.

Create an additional function that constructs default options common to
all DHCP messages.

Set the DHCP Client ID option as noticed by Grant Erickson in ConnMan
commit b18d9798b3a0ae46ed87d6d2be8d5a474bf3ab1e:

"Some Internet gateways and Wi-Fi access points are unhappy when the
DHCPv4 client-id option (61) is missing and will refuse to issue a
DHCP lease."
---
src/dhcp/client.c | 163 +++++++++++++++++++++++++++++++++++++++++++++++++++
src/dhcp/client.h | 3 +
src/dhcp/protocol.h | 4 ++
3 files changed, 170 insertions(+)

diff --git a/src/dhcp/client.c b/src/dhcp/client.c
index 172ddd9..1d4d957 100644
--- a/src/dhcp/client.c
+++ b/src/dhcp/client.c
@@ -21,19 +21,25 @@
#include <errno.h>
#include <string.h>
#include <stdio.h>
+#include <net/ethernet.h>

#include "util.h"
#include "list.h"

#include "protocol.h"
+#include "internal.h"
#include "client.h"

+#define DHCP_CLIENT_MIN_OPTIONS_SIZE 312
+
struct DHCPClient {
DHCPState state;
int index;
uint8_t *req_opts;
int req_opts_size;
struct in_addr *last_addr;
+ struct ether_addr mac_addr;
+ uint32_t xid;
};

static uint8_t default_req_opts[] = {
@@ -114,6 +120,163 @@ int dhcp_client_set_index(DHCPClient *client, int interface_index)
return 0;
}

+int dhcp_client_set_mac(DHCPClient *client, struct ether_addr *addr)
+{
+ if (!client)
+ return -EINVAL;
+
+ if (client->state != DHCP_STATE_INIT)
+ return -EBUSY;
+
+ memcpy(&client->mac_addr, addr, ETH_ALEN);
+
+ return 0;
+}
+
+static int client_packet_init(DHCPClient *client, uint8_t type,
+ DHCPMessage *message, uint8_t **opt,
+ int *optlen)
+{
+ int err;
+
+ *opt = (uint8_t *)(message + 1);
+
+ *optlen -= 4;
+ if (*optlen < 0)
+ return -ENOBUFS;
+
+ message->op = BOOTREQUEST;
+ message->htype = 1;
+ message->hlen = ETHER_ADDR_LEN;
+ message->xid = htonl(client->xid);
+
+ memcpy(&message->chaddr, &client->mac_addr, ETH_ALEN);
+ (*opt)[0] = 0x63;
+ (*opt)[1] = 0x82;
+ (*opt)[2] = 0x53;
+ (*opt)[3] = 0x63;
+
+ *opt += 4;
+
+ err = __dhcp_option_append(opt, optlen, DHCP_OPTION_MESSAGE_TYPE,
+ 1, &type);
+ if (err < 0)
+ return err;
+
+ err = __dhcp_option_append(opt, optlen,
+ DHCP_OPTION_CLIENT_IDENTIFIER,
+ ETH_ALEN, &client->mac_addr);
+ if (err < 0)
+ return err;
+
+ if (type == DHCP_DISCOVER || type == DHCP_REQUEST) {
+ err = __dhcp_option_append(opt, optlen,
+ DHCP_OPTION_PARAMETER_REQUEST_LIST,
+ client->req_opts_size,
+ client->req_opts);
+ if (err < 0)
+ return err;
+ }
+
+ return 0;
+}
+
+static uint16_t client_checksum(void *buf, int len)
+{
+ uint32_t sum;
+ uint16_t *check;
+ int i;
+ uint8_t *odd;
+
+ sum = 0;
+ check = buf;
+
+ for (i = 0; i < len / 2 ; i++)
+ sum += check[i];
+
+ if (len & 0x01) {
+ odd = buf;
+ sum += odd[len];
+ }
+
+ return ~((sum & 0xffff) + (sum >> 16));
+}
+
+static int client_send_discover(DHCPClient *client)
+{
+ int err = 0;
+ DHCPPacket *discover;
+ int optlen, len;
+ uint8_t *opt;
+
+ optlen = DHCP_CLIENT_MIN_OPTIONS_SIZE;
+ len = sizeof(DHCPPacket) + optlen;
+
+ discover = malloc0(len);
+
+ if (!discover)
+ return -ENOBUFS;
+
+ err = client_packet_init(client, DHCP_DISCOVER, &discover->dhcp,
+ &opt, &optlen);
+ if (err < 0)
+ goto error;
+
+ if (client->last_addr) {
+ err = __dhcp_option_append(&opt, &optlen,
+ DHCP_OPTION_REQUESTED_IP_ADDRESS,
+ 4, &client->last_addr->s_addr);
+ if (err < 0)
+ goto error;
+ }
+
+ err = __dhcp_option_append(&opt, &optlen, DHCP_OPTION_END, 0, NULL);
+ if (err < 0)
+ goto error;
+
+ discover->ip.version = IPVERSION;
+ discover->ip.ihl = sizeof(discover->ip) >> 2;
+ discover->ip.tot_len = htons(len);
+
+ discover->ip.protocol = IPPROTO_UDP;
+ discover->ip.saddr = INADDR_ANY;
+ discover->ip.daddr = INADDR_BROADCAST;
+
+ discover->udp.source = htons(DHCP_PORT_CLIENT);
+ discover->udp.dest = htons(DHCP_PORT_SERVER);
+ discover->udp.len = htons(len - sizeof(discover->ip));
+
+ discover->ip.check = discover->udp.len;
+ discover->udp.check = client_checksum(&discover->ip.ttl,
+ len - 8);
+
+ discover->ip.ttl = IPDEFTTL;
+ discover->ip.check = 0;
+ discover->ip.check = client_checksum(&discover->ip,
+ sizeof(discover->ip));
+
+ err = __dhcp_network_send_raw_packet(client->index, discover, len);
+
+error:
+ free(discover);
+
+ return err;
+}
+
+int dhcp_client_start(DHCPClient *client)
+{
+ if (!client || client->index == -1)
+ return -EINVAL;
+
+ if (client->state != DHCP_STATE_INIT &&
+ client->state != DHCP_STATE_INIT_REBOOT)
+ return -EBUSY;
+
+ client->xid = random_u();
+
+ return client_send_discover(client);
+}
+
DHCPClient *dhcp_client_new(void)
{
DHCPClient *client;
diff --git a/src/dhcp/client.h b/src/dhcp/client.h
index 447c03b..fb7682a 100644
--- a/src/dhcp/client.h
+++ b/src/dhcp/client.h
@@ -22,6 +22,7 @@
***/

#include <netinet/in.h>
+#include <net/ethernet.h>

struct DHCPClient;
typedef struct DHCPClient DHCPClient;
@@ -30,5 +31,7 @@ int dhcp_client_set_request_option(DHCPClient *client, uint8_t option);
int dhcp_client_set_request_address(DHCPClient *client,
struct in_addr *last_address);
int dhcp_client_set_index(DHCPClient *client, int interface_index);
+int dhcp_client_set_mac(DHCPClient *client, struct ether_addr *addr);

+int dhcp_client_start(DHCPClient *client);
DHCPClient *dhcp_client_new(void);
diff --git a/src/dhcp/protocol.h b/src/dhcp/protocol.h
index 3fc4baf..a2c5216 100644
--- a/src/dhcp/protocol.h
+++ b/src/dhcp/protocol.h
@@ -52,6 +52,9 @@ struct DHCPPacket {

typedef struct DHCPPacket DHCPPacket;

+#define DHCP_PORT_SERVER 67
+#define DHCP_PORT_CLIENT 68
+
enum DHCPState {
DHCP_STATE_INIT = 0,
DHCP_STATE_SELECTING = 1,
@@ -90,4 +93,5 @@ typedef enum DHCPState DHCPState;
#define DHCP_OPTION_OVERLOAD 52
#define DHCP_OPTION_MESSAGE_TYPE 53
#define DHCP_OPTION_PARAMETER_REQUEST_LIST 55
+#define DHCP_OPTION_CLIENT_IDENTIFIER 61
#define DHCP_OPTION_END 255
--
1.7.10.4
Lennart Poettering
2013-11-14 00:17:55 UTC
Permalink
Post by Patrik Flykt
+static int client_send_discover(DHCPClient *client)
+{
+ int err = 0;
+ DHCPPacket *discover;
+ int optlen, len;
+ uint8_t *opt;
+
+ optlen = DHCP_CLIENT_MIN_OPTIONS_SIZE;
+ len = sizeof(DHCPPacket) + optlen;
+
+ discover = malloc0(len);
+ if (!discover)
+ return -ENOBUFS;
Please use -ENOMEM for OOM errors
Post by Patrik Flykt
+ err = __dhcp_network_send_raw_packet(client->index, discover, len);
+
+ free(discover);
+
+ return err;
_cleanup_free_ is your friend and the best thing since sliced bread. It
allows you to get rid of all gotos here!

Lennart
--
Lennart Poettering, Red Hat
Patrik Flykt
2013-11-13 21:22:53 UTC
Permalink
Compute the default T1 and T2 timer values if they were not set by
the DHCP server. Verify that the values are reasonable.
---
src/dhcp/client.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++-
src/dhcp/protocol.h | 2 +
2 files changed, 124 insertions(+), 1 deletion(-)

diff --git a/src/dhcp/client.c b/src/dhcp/client.c
index 669804a..50008a9 100644
--- a/src/dhcp/client.c
+++ b/src/dhcp/client.c
@@ -34,6 +34,8 @@

struct DHCPLease {
uint32_t lifetime;
+ uint32_t t2;
+ uint32_t t1;
struct in_addr address;
struct in_addr server_address;
struct in_addr subnet_mask;
@@ -57,6 +59,10 @@ struct DHCPClient {
uint32_t xid;
uint64_t start_time;
unsigned int attempt;
+ uint64_t request_sent;
+ sd_event_source *timeout_expire;
+ sd_event_source *timeout_t1;
+ sd_event_source *timeout_t2;
DHCPLease *lease;
};

@@ -177,6 +183,16 @@ static int client_stop(DHCPClient *client, int error)
client->timeout_resend =
sd_event_source_unref(client->timeout_resend);

+ if (client->timeout_expire)
+ client->timeout_expire =
+ sd_event_source_unref(client->timeout_expire);
+ if (client->timeout_t2)
+ client->timeout_t2 =
+ sd_event_source_unref(client->timeout_t2);
+ if (client->timeout_t1)
+ client->timeout_t1 =
+ sd_event_source_unref(client->timeout_t1);
+
client->attempt = 1;

switch (client->state) {
@@ -184,6 +200,7 @@ static int client_stop(DHCPClient *client, int error)
case DHCP_STATE_INIT:
case DHCP_STATE_SELECTING:
case DHCP_STATE_REQUESTING:
+ case DHCP_STATE_BOUND:

client->start_time = 0;
client->state = DHCP_STATE_INIT;
@@ -191,7 +208,6 @@ static int client_stop(DHCPClient *client, int error)

case DHCP_STATE_INIT_REBOOT:
case DHCP_STATE_REBOOTING:
- case DHCP_STATE_BOUND:
case DHCP_STATE_RENEWING:
case DHCP_STATE_REBINDING:

@@ -447,6 +463,8 @@ static int client_timeout_resend(sd_event_source *s, uint64_t usec,
if (err < 0 && client->attempt >= 64)
goto error;

+ client->request_sent = usec;
+
break;

case DHCP_STATE_INIT_REBOOT:
@@ -466,6 +484,22 @@ error:
return 0;
}

+static int client_timeout_expire(sd_event_source *s, uint64_t usec,
+ void *userdata)
+{
+ return 0;
+}
+
+static int client_timeout_t2(sd_event_source *s, uint64_t usec, void *userdata)
+{
+ return 0;
+}
+
+static int client_timeout_t1(sd_event_source *s, uint64_t usec, void *userdata)
+{
+ return 0;
+}
+
static int client_parse_offer(uint8_t code, uint8_t len, uint8_t *option,
void *user_data)
{
@@ -499,6 +533,22 @@ static int client_parse_offer(uint8_t code, uint8_t len, uint8_t *option,
memcpy(&lease->router.s_addr, option, 4);

break;
+
+ case DHCP_OPTION_RENEWAL_T1_TIME:
+ if (len == 4) {
+ memcpy(&val, option, 4);
+ lease->t1 = ntohl(val);
+ }
+
+ break;
+
+ case DHCP_OPTION_REBINDING_T2_TIME:
+ if (len == 4) {
+ memcpy(&val, option, 4);
+ lease->t2 = ntohl(val);
+ }
+
+ break;
}

return 0;
@@ -635,6 +685,72 @@ error:
return err;
}

+static uint64_t client_compute_timeout(uint64_t request_sent,
+ uint32_t lifetime)
+{
+ return request_sent + (lifetime - 3) * USEC_PER_SEC +
+ + (random_u() & 0x1fffff);
+}
+
+static int client_set_lease_timeouts(DHCPClient *client, uint64_t usec)
+{
+ int err;
+ uint64_t next_timeout;
+
+ if (client->lease->lifetime < 10)
+ return -EINVAL;
+
+ if (!client->lease->t1)
+ client->lease->t1 = client->lease->lifetime / 2;
+
+ next_timeout = client_compute_timeout(client->request_sent,
+ client->lease->t1);
+ if (next_timeout < usec)
+ return -EINVAL;
+
+ err = sd_event_add_monotonic(client->event, next_timeout,
+ 10 * USEC_PER_MSEC,
+ client_timeout_t1, client,
+ &client->timeout_t1);
+ if (err < 0)
+ return err;
+
+ if (!client->lease->t2)
+ client->lease->t2 = client->lease->lifetime * 7 / 8;
+
+ if (client->lease->t2 < client->lease->t1)
+ return -EINVAL;
+
+ if (client->lease->lifetime < client->lease->t2)
+ return -EINVAL;
+
+ next_timeout = client_compute_timeout(client->request_sent,
+ client->lease->t2);
+ if (next_timeout < usec)
+ return -EINVAL;
+
+ err = sd_event_add_monotonic(client->event, next_timeout,
+ 10 * USEC_PER_MSEC,
+ client_timeout_t2, client,
+ &client->timeout_t2);
+ if (err < 0)
+ return err;
+
+ next_timeout = client_compute_timeout(client->request_sent,
+ client->lease->lifetime);
+ if (next_timeout < usec)
+ return -EINVAL;
+
+ err = sd_event_add_monotonic(client->event, next_timeout,
+ 10 * USEC_PER_MSEC,
+ client_timeout_expire, client,
+ &client->timeout_expire);
+ if (err < 0)
+ return err;
+
+ return 0;
+}
+
static int client_receive_raw_message(sd_event_source *s, int fd,
uint32_t revents, void *userdata)
{
@@ -697,6 +813,11 @@ static int client_receive_raw_message(sd_event_source *s, int fd,
client->last_addr->s_addr =
client->lease->address.s_addr;

+ err = client_set_lease_timeouts(client,
+ now(CLOCK_MONOTONIC));
+ if (err < 0 )
+ goto error;
+
client_notify(client, DHCP_EVENT_IP_ACQUIRE);

close(client->fd);
diff --git a/src/dhcp/protocol.h b/src/dhcp/protocol.h
index 6503931..773c29c 100644
--- a/src/dhcp/protocol.h
+++ b/src/dhcp/protocol.h
@@ -100,5 +100,7 @@ typedef enum DHCPState DHCPState;
#define DHCP_OPTION_SERVER_IDENTIFIER 54
#define DHCP_OPTION_PARAMETER_REQUEST_LIST 55
#define DHCP_OPTION_MAXIMUM_MESSAGE_SIZE 57
+#define DHCP_OPTION_RENEWAL_T1_TIME 58
+#define DHCP_OPTION_REBINDING_T2_TIME 59
#define DHCP_OPTION_CLIENT_IDENTIFIER 61
#define DHCP_OPTION_END 255
--
1.7.10.4
Zbigniew Jędrzejewski-Szmek
2013-11-14 01:27:34 UTC
Permalink
Post by Patrik Flykt
Compute the default T1 and T2 timer values if they were not set by
the DHCP server. Verify that the values are reasonable.
---
src/dhcp/client.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++-
src/dhcp/protocol.h | 2 +
2 files changed, 124 insertions(+), 1 deletion(-)
diff --git a/src/dhcp/client.c b/src/dhcp/client.c
index 669804a..50008a9 100644
--- a/src/dhcp/client.c
+++ b/src/dhcp/client.c
@@ -34,6 +34,8 @@
struct DHCPLease {
uint32_t lifetime;
+ uint32_t t2;
+ uint32_t t1;
Can we have the order t1, t2 everywhere?
Post by Patrik Flykt
struct in_addr address;
struct in_addr server_address;
struct in_addr subnet_mask;
@@ -57,6 +59,10 @@ struct DHCPClient {
uint32_t xid;
uint64_t start_time;
unsigned int attempt;
+ uint64_t request_sent;
+ sd_event_source *timeout_expire;
+ sd_event_source *timeout_t1;
+ sd_event_source *timeout_t2;
DHCPLease *lease;
};
@@ -177,6 +183,16 @@ static int client_stop(DHCPClient *client, int error)
client->timeout_resend =
sd_event_source_unref(client->timeout_resend);
+ if (client->timeout_expire)
+ client->timeout_expire =
+ sd_event_source_unref(client->timeout_expire);
sd_event_source_unref accepts NULL, so those if's are not necessary.
I think that this is not performance trical code, so it's better to
simplify.
Post by Patrik Flykt
+ if (client->timeout_t2)
+ client->timeout_t2 =
+ sd_event_source_unref(client->timeout_t2);
+ if (client->timeout_t1)
+ client->timeout_t1 =
+ sd_event_source_unref(client->timeout_t1);
+
client->attempt = 1;
switch (client->state) {
@@ -184,6 +200,7 @@ static int client_stop(DHCPClient *client, int error)
client->start_time = 0;
client->state = DHCP_STATE_INIT;
@@ -191,7 +208,6 @@ static int client_stop(DHCPClient *client, int error)
@@ -447,6 +463,8 @@ static int client_timeout_resend(sd_event_source *s, uint64_t usec,
if (err < 0 && client->attempt >= 64)
goto error;
+ client->request_sent = usec;
+
break;
return 0;
}
+static int client_timeout_expire(sd_event_source *s, uint64_t usec,
+ void *userdata)
+{
+ return 0;
+}
+
+static int client_timeout_t2(sd_event_source *s, uint64_t usec, void *userdata)
+{
+ return 0;
+}
+
+static int client_timeout_t1(sd_event_source *s, uint64_t usec, void *userdata)
+{
+ return 0;
+}
+
static int client_parse_offer(uint8_t code, uint8_t len, uint8_t *option,
void *user_data)
{
@@ -499,6 +533,22 @@ static int client_parse_offer(uint8_t code, uint8_t len, uint8_t *option,
memcpy(&lease->router.s_addr, option, 4);
break;
+
+ if (len == 4) {
+ memcpy(&val, option, 4);
+ lease->t1 = ntohl(val);
+ }
+
+ break;
+
+ if (len == 4) {
+ memcpy(&val, option, 4);
+ lease->t2 = ntohl(val);
+ }
+
+ break;
}
return 0;
return err;
}
+static uint64_t client_compute_timeout(uint64_t request_sent,
+ uint32_t lifetime)
+{
+ return request_sent + (lifetime - 3) * USEC_PER_SEC +
+ + (random_u() & 0x1fffff);
+}
+
+static int client_set_lease_timeouts(DHCPClient *client, uint64_t usec)
+{
+ int err;
+ uint64_t next_timeout;
+
+ if (client->lease->lifetime < 10)
+ return -EINVAL;
+
+ if (!client->lease->t1)
+ client->lease->t1 = client->lease->lifetime / 2;
+
+ next_timeout = client_compute_timeout(client->request_sent,
+ client->lease->t1);
+ if (next_timeout < usec)
+ return -EINVAL;
+
+ err = sd_event_add_monotonic(client->event, next_timeout,
+ 10 * USEC_PER_MSEC,
+ client_timeout_t1, client,
+ &client->timeout_t1);
+ if (err < 0)
+ return err;
+
+ if (!client->lease->t2)
+ client->lease->t2 = client->lease->lifetime * 7 / 8;
+
+ if (client->lease->t2 < client->lease->t1)
+ return -EINVAL;
+
+ if (client->lease->lifetime < client->lease->t2)
+ return -EINVAL;
+
+ next_timeout = client_compute_timeout(client->request_sent,
+ client->lease->t2);
+ if (next_timeout < usec)
+ return -EINVAL;
+
+ err = sd_event_add_monotonic(client->event, next_timeout,
+ 10 * USEC_PER_MSEC,
+ client_timeout_t2, client,
+ &client->timeout_t2);
+ if (err < 0)
+ return err;
+
+ next_timeout = client_compute_timeout(client->request_sent,
+ client->lease->lifetime);
+ if (next_timeout < usec)
+ return -EINVAL;
+
+ err = sd_event_add_monotonic(client->event, next_timeout,
+ 10 * USEC_PER_MSEC,
+ client_timeout_expire, client,
+ &client->timeout_expire);
+ if (err < 0)
+ return err;
+
+ return 0;
+}
+
static int client_receive_raw_message(sd_event_source *s, int fd,
uint32_t revents, void *userdata)
{
@@ -697,6 +813,11 @@ static int client_receive_raw_message(sd_event_source *s, int fd,
client->last_addr->s_addr =
client->lease->address.s_addr;
+ err = client_set_lease_timeouts(client,
+ now(CLOCK_MONOTONIC));
+ if (err < 0 )
+ goto error;
+
client_notify(client, DHCP_EVENT_IP_ACQUIRE);
close(client->fd);
diff --git a/src/dhcp/protocol.h b/src/dhcp/protocol.h
index 6503931..773c29c 100644
--- a/src/dhcp/protocol.h
+++ b/src/dhcp/protocol.h
@@ -100,5 +100,7 @@ typedef enum DHCPState DHCPState;
#define DHCP_OPTION_SERVER_IDENTIFIER 54
#define DHCP_OPTION_PARAMETER_REQUEST_LIST 55
#define DHCP_OPTION_MAXIMUM_MESSAGE_SIZE 57
+#define DHCP_OPTION_RENEWAL_T1_TIME 58
+#define DHCP_OPTION_REBINDING_T2_TIME 59
#define DHCP_OPTION_CLIENT_IDENTIFIER 61
#define DHCP_OPTION_END 255
Zbyszek
Patrik Flykt
2013-11-13 21:22:52 UTC
Permalink
Process a DHCP Ack/Nak in much the same way as an DHCP Offer. Factor
out header verification and process options sent. Add notification
functionality with discrete values for the outcome of the DHCP Ack/
Nak processing.
---
src/dhcp/client.c | 145 +++++++++++++++++++++++++++++++++++++++++++++--------
src/dhcp/client.h | 4 ++
2 files changed, 128 insertions(+), 21 deletions(-)

diff --git a/src/dhcp/client.c b/src/dhcp/client.c
index 45c62f3..669804a 100644
--- a/src/dhcp/client.c
+++ b/src/dhcp/client.c
@@ -151,6 +151,11 @@ int dhcp_client_set_mac(DHCPClient *client, struct ether_addr *addr)
return 0;
}

+static int client_notify(DHCPClient *client, int event)
+{
+ return 0;
+}
+
static int client_stop(DHCPClient *client, int error)
{
if (!client)
@@ -178,6 +183,7 @@ static int client_stop(DHCPClient *client, int error)

case DHCP_STATE_INIT:
case DHCP_STATE_SELECTING:
+ case DHCP_STATE_REQUESTING:

client->start_time = 0;
client->state = DHCP_STATE_INIT;
@@ -185,7 +191,6 @@ static int client_stop(DHCPClient *client, int error)

case DHCP_STATE_INIT_REBOOT:
case DHCP_STATE_REBOOTING:
- case DHCP_STATE_REQUESTING:
case DHCP_STATE_BOUND:
case DHCP_STATE_RENEWING:
case DHCP_STATE_REBINDING:
@@ -499,41 +504,52 @@ static int client_parse_offer(uint8_t code, uint8_t len, uint8_t *option,
return 0;
}

-static int client_receive_offer(DHCPClient *client,
- DHCPPacket *offer, int len)
+static int client_verify_headers(DHCPClient *client,
+ DHCPPacket *message, int len)
{
int hdrlen;
- DHCPLease *lease;

if (len < (DHCP_IP_UDP_SIZE + DHCP_MESSAGE_SIZE))
return -EINVAL;

- hdrlen = offer->ip.ihl * 4;
- if (hdrlen < 20 || hdrlen > len || client_checksum(&offer->ip,
+ hdrlen = message->ip.ihl * 4;
+ if (hdrlen < 20 || hdrlen > len || client_checksum(&message->ip,
hdrlen))
return -EINVAL;

- offer->ip.check = offer->udp.len;
- offer->ip.ttl = 0;
+ message->ip.check = message->udp.len;
+ message->ip.ttl = 0;

- if (hdrlen + ntohs(offer->udp.len) > len ||
- client_checksum(&offer->ip.ttl, ntohs(offer->udp.len) + 12))
+ if (hdrlen + ntohs(message->udp.len) > len ||
+ client_checksum(&message->ip.ttl, ntohs(message->udp.len) + 12))
return -EINVAL;

- if (ntohs(offer->udp.source) != DHCP_PORT_SERVER ||
- ntohs(offer->udp.dest) != DHCP_PORT_CLIENT)
+ if (ntohs(message->udp.source) != DHCP_PORT_SERVER ||
+ ntohs(message->udp.dest) != DHCP_PORT_CLIENT)
return -EINVAL;

- if (offer->dhcp.op != BOOTREPLY)
+ if (message->dhcp.op != BOOTREPLY)
return -EINVAL;

- if (ntohl(offer->dhcp.xid) != client->xid)
+ if (ntohl(message->dhcp.xid) != client->xid)
return -EINVAL;

- if (memcmp(&offer->dhcp.chaddr[0], &client->mac_addr.ether_addr_octet,
+ if (memcmp(&message->dhcp.chaddr[0], &client->mac_addr.ether_addr_octet,
ETHER_ADDR_LEN))
return -EINVAL;

+ return 0;
+}
+
+static int client_receive_offer(DHCPClient *client, DHCPPacket *offer, int len)
+{
+ int err;
+ DHCPLease *lease;
+
+ err = client_verify_headers(client, offer, len);
+ if (err < 0)
+ return err;
+
lease = new0(DHCPLease, 1);
if (!lease)
return -ENOBUFS;
@@ -561,6 +577,64 @@ error:
return -ENOMSG;
}

+static int client_receive_ack(DHCPClient *client, DHCPPacket *offer, int len)
+{
+ int err = -ENOMSG;
+ DHCPLease *lease;
+ int type;
+
+ err = client_verify_headers(client, offer, len);
+ if (err < 0)
+ return err;
+
+ lease = new0(DHCPLease, 1);
+ if (!lease)
+ return -ENOBUFS;
+
+ len = len - DHCP_IP_UDP_SIZE;
+ type = __dhcp_option_parse(&offer->dhcp, len, client_parse_offer,
+ lease);
+
+ if (type == DHCP_NAK) {
+ err = DHCP_EVENT_NAK;
+ goto error;
+ }
+
+ if (type != DHCP_ACK)
+ goto error;
+
+ lease->address.s_addr = offer->dhcp.yiaddr;
+
+ if (lease->address.s_addr == INADDR_ANY ||
+ lease->server_address.s_addr == INADDR_ANY ||
+ lease->subnet_mask.s_addr == INADDR_ANY ||
+ lease->lifetime == 0) {
+ err = -ENOMSG;
+ goto error;
+ }
+
+ err = 0;
+
+ if (client->lease) {
+ if (client->lease->address.s_addr != lease->address.s_addr ||
+ client->lease->subnet_mask.s_addr !=
+ lease->subnet_mask.s_addr ||
+ client->lease->router.s_addr != lease->router.s_addr) {
+ err = DHCP_EVENT_IP_CHANGE;
+ }
+ free(client->lease);
+ }
+
+ client->lease = lease;
+
+ return err;
+
+error:
+ free(lease);
+
+ return err;
+}
+
static int client_receive_raw_message(sd_event_source *s, int fd,
uint32_t revents, void *userdata)
{
@@ -568,6 +642,7 @@ static int client_receive_raw_message(sd_event_source *s, int fd,
int err = 0;
int len, buflen;
uint8_t *buf;
+ char tmp;
DHCPPacket *message;

buflen = sizeof(DHCPPacket) + DHCP_CLIENT_MIN_OPTIONS_SIZE;
@@ -602,10 +677,36 @@ static int client_receive_raw_message(sd_event_source *s, int fd,

break;

+ case DHCP_STATE_REQUESTING:
+
+ err = client_receive_ack(client, message, len);
+ if (err == DHCP_EVENT_NAK)
+ goto error;
+
+ if (err >= 0) {
+ client->timeout_resend =
+ sd_event_source_unref(client->timeout_resend);
+
+ client->state = DHCP_STATE_BOUND;
+ client->attempt = 1;
+
+ if (!client->last_addr)
+ client->last_addr =
+ malloc(sizeof(struct in_addr));
+ if (client->last_addr)
+ client->last_addr->s_addr =
+ client->lease->address.s_addr;
+
+ client_notify(client, DHCP_EVENT_IP_ACQUIRE);
+
+ close(client->fd);
+ client->fd = 0;
+ }
+ break;
+
case DHCP_STATE_INIT:
case DHCP_STATE_INIT_REBOOT:
case DHCP_STATE_REBOOTING:
- case DHCP_STATE_REQUESTING:
case DHCP_STATE_BOUND:
case DHCP_STATE_RENEWING:
case DHCP_STATE_REBINDING:
@@ -618,13 +719,15 @@ static int client_receive_raw_message(sd_event_source *s, int fd,
return 0;

error:
- if (err < 0)
- return client_stop(client, err);
+ if (buf)
+ free(buf);
+ else
+ read(fd, &tmp, 1);

- if (!err)
- read(fd, buf, 1);
+ if (err != 0)
+ return client_stop(client, err);

- return 0;
+ return err;
}

int dhcp_client_start(DHCPClient *client)
diff --git a/src/dhcp/client.h b/src/dhcp/client.h
index 568e70a..5ce7fb4 100644
--- a/src/dhcp/client.h
+++ b/src/dhcp/client.h
@@ -26,6 +26,10 @@

#include "sd-event.h"

+#define DHCP_EVENT_NAK 1
+#define DHCP_EVENT_IP_ACQUIRE 2
+#define DHCP_EVENT_IP_CHANGE 3
+
struct DHCPClient;
typedef struct DHCPClient DHCPClient;
--
1.7.10.4
Zbigniew Jędrzejewski-Szmek
2013-11-14 01:40:29 UTC
Permalink
Post by Patrik Flykt
Process a DHCP Ack/Nak in much the same way as an DHCP Offer. Factor
out header verification and process options sent. Add notification
functionality with discrete values for the outcome of the DHCP Ack/
Nak processing.
---
src/dhcp/client.c | 145 +++++++++++++++++++++++++++++++++++++++++++++--------
src/dhcp/client.h | 4 ++
2 files changed, 128 insertions(+), 21 deletions(-)
diff --git a/src/dhcp/client.c b/src/dhcp/client.c
index 45c62f3..669804a 100644
--- a/src/dhcp/client.c
+++ b/src/dhcp/client.c
@@ -151,6 +151,11 @@ int dhcp_client_set_mac(DHCPClient *client, struct ether_addr *addr)
return 0;
}
+static int client_notify(DHCPClient *client, int event)
+{
+ return 0;
+}
+
static int client_stop(DHCPClient *client, int error)
{
if (!client)
@@ -178,6 +183,7 @@ static int client_stop(DHCPClient *client, int error)
client->start_time = 0;
client->state = DHCP_STATE_INIT;
@@ -185,7 +191,6 @@ static int client_stop(DHCPClient *client, int error)
@@ -499,41 +504,52 @@ static int client_parse_offer(uint8_t code, uint8_t len, uint8_t *option,
return 0;
}
-static int client_receive_offer(DHCPClient *client,
- DHCPPacket *offer, int len)
+static int client_verify_headers(DHCPClient *client,
+ DHCPPacket *message, int len)
{
int hdrlen;
- DHCPLease *lease;
if (len < (DHCP_IP_UDP_SIZE + DHCP_MESSAGE_SIZE))
return -EINVAL;
- hdrlen = offer->ip.ihl * 4;
- if (hdrlen < 20 || hdrlen > len || client_checksum(&offer->ip,
+ hdrlen = message->ip.ihl * 4;
+ if (hdrlen < 20 || hdrlen > len || client_checksum(&message->ip,
hdrlen))
return -EINVAL;
- offer->ip.check = offer->udp.len;
- offer->ip.ttl = 0;
+ message->ip.check = message->udp.len;
+ message->ip.ttl = 0;
- if (hdrlen + ntohs(offer->udp.len) > len ||
- client_checksum(&offer->ip.ttl, ntohs(offer->udp.len) + 12))
+ if (hdrlen + ntohs(message->udp.len) > len ||
+ client_checksum(&message->ip.ttl, ntohs(message->udp.len) + 12))
return -EINVAL;
- if (ntohs(offer->udp.source) != DHCP_PORT_SERVER ||
- ntohs(offer->udp.dest) != DHCP_PORT_CLIENT)
+ if (ntohs(message->udp.source) != DHCP_PORT_SERVER ||
+ ntohs(message->udp.dest) != DHCP_PORT_CLIENT)
return -EINVAL;
- if (offer->dhcp.op != BOOTREPLY)
+ if (message->dhcp.op != BOOTREPLY)
return -EINVAL;
- if (ntohl(offer->dhcp.xid) != client->xid)
+ if (ntohl(message->dhcp.xid) != client->xid)
return -EINVAL;
- if (memcmp(&offer->dhcp.chaddr[0], &client->mac_addr.ether_addr_octet,
+ if (memcmp(&message->dhcp.chaddr[0], &client->mac_addr.ether_addr_octet,
ETHER_ADDR_LEN))
return -EINVAL;
+ return 0;
+}
+
+static int client_receive_offer(DHCPClient *client, DHCPPacket *offer, int len)
+{
+ int err;
+ DHCPLease *lease;
+
+ err = client_verify_headers(client, offer, len);
+ if (err < 0)
+ return err;
+
lease = new0(DHCPLease, 1);
if (!lease)
return -ENOBUFS;
return -ENOMSG;
}
+static int client_receive_ack(DHCPClient *client, DHCPPacket *offer, int len)
+{
+ int err = -ENOMSG;
Since err is not an error always, but a sucess return value too,
it would be nicer to call it r. Also this value is override right below.

You might consider the following pattern:
_cleanup_free_ DHCPLease *lease = NULL;

and ...
Post by Patrik Flykt
+ DHCPLease *lease;
+ int type;
+
+ err = client_verify_headers(client, offer, len);
+ if (err < 0)
+ return err;
+
+ lease = new0(DHCPLease, 1);
+ if (!lease)
+ return -ENOBUFS;
+
+ len = len - DHCP_IP_UDP_SIZE;
+ type = __dhcp_option_parse(&offer->dhcp, len, client_parse_offer,
+ lease);
+
+ if (type == DHCP_NAK) {
+ err = DHCP_EVENT_NAK;
+ goto error;
... and
return DHCP_EVENT_NAK;
and ...
Post by Patrik Flykt
+ }
+
+ if (type != DHCP_ACK)
+ goto error;
Is err set correctly here?
Post by Patrik Flykt
+
+ lease->address.s_addr = offer->dhcp.yiaddr;
+
+ if (lease->address.s_addr == INADDR_ANY ||
+ lease->server_address.s_addr == INADDR_ANY ||
+ lease->subnet_mask.s_addr == INADDR_ANY ||
+ lease->lifetime == 0) {
+ err = -ENOMSG;
+ goto error;
... and just return here ...
Post by Patrik Flykt
+ }
+
+ err = 0;
+
+ if (client->lease) {
+ if (client->lease->address.s_addr != lease->address.s_addr ||
+ client->lease->subnet_mask.s_addr !=
+ lease->subnet_mask.s_addr ||
+ client->lease->router.s_addr != lease->router.s_addr) {
+ err = DHCP_EVENT_IP_CHANGE;
+ }
+ free(client->lease);
+ }
+
+ client->lease = lease;
... and here
lease = NULL;

and ...
Post by Patrik Flykt
+
+ return err;
... and remove the part below.
Post by Patrik Flykt
+
+ free(lease);
+
+ return err;
+}
+
static int client_receive_raw_message(sd_event_source *s, int fd,
uint32_t revents, void *userdata)
{
@@ -568,6 +642,7 @@ static int client_receive_raw_message(sd_event_source *s, int fd,
int err = 0;
int len, buflen;
uint8_t *buf;
+ char tmp;
DHCPPacket *message;
buflen = sizeof(DHCPPacket) + DHCP_CLIENT_MIN_OPTIONS_SIZE;
@@ -602,10 +677,36 @@ static int client_receive_raw_message(sd_event_source *s, int fd,
break;
+
+ err = client_receive_ack(client, message, len);
+ if (err == DHCP_EVENT_NAK)
+ goto error;
+
+ if (err >= 0) {
+ client->timeout_resend =
+ sd_event_source_unref(client->timeout_resend);
+
+ client->state = DHCP_STATE_BOUND;
+ client->attempt = 1;
+
+ if (!client->last_addr)
+ client->last_addr =
+ malloc(sizeof(struct in_addr));
+ if (client->last_addr)
+ client->last_addr->s_addr =
+ client->lease->address.s_addr;
+
+ client_notify(client, DHCP_EVENT_IP_ACQUIRE);
+
+ close(client->fd);
+ client->fd = 0;
-1 not 0.
Post by Patrik Flykt
+ }
+ break;
+
@@ -618,13 +719,15 @@ static int client_receive_raw_message(sd_event_source *s, int fd,
return 0;
- if (err < 0)
- return client_stop(client, err);
+ if (buf)
+ free(buf);
+ else
+ read(fd, &tmp, 1);
- if (!err)
- read(fd, buf, 1);
+ if (err != 0)
+ return client_stop(client, err);
- return 0;
+ return err;
}
int dhcp_client_start(DHCPClient *client)
diff --git a/src/dhcp/client.h b/src/dhcp/client.h
index 568e70a..5ce7fb4 100644
--- a/src/dhcp/client.h
+++ b/src/dhcp/client.h
@@ -26,6 +26,10 @@
#include "sd-event.h"
+#define DHCP_EVENT_NAK 1
+#define DHCP_EVENT_IP_ACQUIRE 2
+#define DHCP_EVENT_IP_CHANGE 3
+
struct DHCPClient;
typedef struct DHCPClient DHCPClient;
Zbyszek
Patrik Flykt
2013-11-13 21:22:34 UTC
Permalink
Create an initial simple test program for these two cases.
---
src/dhcp/test-dhcp-option.c | 54 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 54 insertions(+)
create mode 100644 src/dhcp/test-dhcp-option.c

diff --git a/src/dhcp/test-dhcp-option.c b/src/dhcp/test-dhcp-option.c
new file mode 100644
index 0000000..25cb17c
--- /dev/null
+++ b/src/dhcp/test-dhcp-option.c
@@ -0,0 +1,54 @@
+
+#include <stdio.h>
+#include <stdbool.h>
+#include <errno.h>
+#include <string.h>
+#include <assert.h>
+
+#include <util.h>
+
+#include "protocol.h"
+#include "internal.h"
+
+static void test_invalid_buffer_length(void)
+{
+ struct dhcp_message message;
+
+ assert(__dhcp_option_parse(&message, INT32_MIN, NULL, NULL) ==
+ -EINVAL);
+ assert(__dhcp_option_parse(&message, sizeof(struct dhcp_message),
+ NULL, NULL) == -EINVAL);
+}
+
+static void test_cookie(void)
+{
+ struct dhcp_message *message;
+ uint32_t len = sizeof(struct dhcp_message) + 4;
+ uint8_t *opt;
+
+ message = malloc0(len);
+
+ opt = (uint8_t *) message + 1;
+ opt[0] = 0xff;
+
+ assert(__dhcp_option_parse(message, len, NULL, NULL) == -EINVAL);
+
+ opt[0] = 99;
+ opt[1] = 130;
+ opt[2] = 83;
+ opt[3] = 99;
+
+ assert(__dhcp_option_parse(message, len, NULL, NULL) == -ENOMSG);
+
+ free(message);
+}
+
+int main(int argc, char *argv[])
+{
+ unsigned int i;
+
+ test_invalid_buffer_length();
+ test_cookie();
+
+ return 0;
+}
--
1.7.10.4
Patrik Flykt
2013-11-13 21:22:38 UTC
Permalink
---
src/dhcp/test-dhcp-client.c | 40 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 40 insertions(+)

diff --git a/src/dhcp/test-dhcp-client.c b/src/dhcp/test-dhcp-client.c
index 26857b6..94f576a 100644
--- a/src/dhcp/test-dhcp-client.c
+++ b/src/dhcp/test-dhcp-client.c
@@ -22,6 +22,7 @@
#include <stdlib.h>
#include <assert.h>
#include <errno.h>
+#include <stdio.h>

#include "protocol.h"
#include "client.h"
@@ -73,9 +74,48 @@ static void test_request_basic(void)
assert(dhcp_client_set_request_option(client, 44) == 0);
}

+static uint16_t client_checksum(void *buf, int len)
+{
+ uint32_t sum;
+ uint16_t *check;
+ int i;
+ uint8_t *odd;
+
+ sum = 0;
+ check = buf;
+
+ for (i = 0; i < len / 2 ; i++)
+ sum += check[i];
+
+ if (len & 0x01) {
+ odd = buf;
+ sum += odd[len];
+ }
+
+ return ~((sum & 0xffff) + (sum >> 16));
+}
+
+static void test_checksum(void)
+{
+ uint8_t buf[20] = {
+ 0x45, 0x00, 0x02, 0x40, 0x00, 0x00, 0x00, 0x00,
+ 0x40, 0x11, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0xff, 0xff, 0xff, 0xff
+ };
+
+ uint8_t check[2] = {
+ 0x78, 0xae
+ };
+
+ uint16_t *val = (uint16_t *)check;
+
+ assert(client_checksum(&buf, 20) == *val);
+}
+
int main(int argc, char *argv[])
{
test_request_basic();
+ test_checksum();

return 0;
}
--
1.7.10.4
Zbigniew Jędrzejewski-Szmek
2013-11-14 02:02:42 UTC
Permalink
Post by Patrik Flykt
---
src/dhcp/test-dhcp-client.c | 40 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 40 insertions(+)
diff --git a/src/dhcp/test-dhcp-client.c b/src/dhcp/test-dhcp-client.c
index 26857b6..94f576a 100644
--- a/src/dhcp/test-dhcp-client.c
+++ b/src/dhcp/test-dhcp-client.c
@@ -22,6 +22,7 @@
#include <stdlib.h>
#include <assert.h>
#include <errno.h>
+#include <stdio.h>
#include "protocol.h"
#include "client.h"
@@ -73,9 +74,48 @@ static void test_request_basic(void)
assert(dhcp_client_set_request_option(client, 44) == 0);
}
+static uint16_t client_checksum(void *buf, int len)
+{
+ uint32_t sum;
= 0;
Post by Patrik Flykt
+ uint16_t *check;
= buf;
Post by Patrik Flykt
+ int i;
+ uint8_t *odd;
+
+ sum = 0;
+ check = buf;
+
+ for (i = 0; i < len / 2 ; i++)
+ sum += check[i];
+
+ if (len & 0x01) {
+ odd = buf;
+ sum += odd[len];
+ }
+
+ return ~((sum & 0xffff) + (sum >> 16));
+}
+
+static void test_checksum(void)
+{
+ uint8_t buf[20] = {
+ 0x45, 0x00, 0x02, 0x40, 0x00, 0x00, 0x00, 0x00,
+ 0x40, 0x11, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0xff, 0xff, 0xff, 0xff
+ };
+
+ uint8_t check[2] = {
+ 0x78, 0xae
+ };
+
+ uint16_t *val = (uint16_t *)check;
No need to cast here.
Post by Patrik Flykt
+
+ assert(client_checksum(&buf, 20) == *val);
+}
+
int main(int argc, char *argv[])
{
test_request_basic();
+ test_checksum();
return 0;
}
Zbyszek
Patrik Flykt
2013-11-13 21:22:50 UTC
Permalink
Create and send a DHCP Request message reusing already existing parts
of the code. This causes factoring out IP and UDP header creation and
moving next timeout calculation to be done every time in the timer
callback function independent of DHCP state. Also add an exponential
part to the timer calculation, bail out if there are errors while
resending the DHCP message for the sixth or more times.
---
src/dhcp/client.c | 162 +++++++++++++++++++++++++++++++++++++++--------------
1 file changed, 120 insertions(+), 42 deletions(-)

diff --git a/src/dhcp/client.c b/src/dhcp/client.c
index 84d5c25..4b316dc 100644
--- a/src/dhcp/client.c
+++ b/src/dhcp/client.c
@@ -56,6 +56,7 @@ struct DHCPClient {
struct ether_addr mac_addr;
uint32_t xid;
uint64_t start_time;
+ unsigned int attempt;
DHCPLease *lease;
};

@@ -171,6 +172,8 @@ static int client_stop(DHCPClient *client, int error)
client->timeout_resend =
sd_event_source_unref(client->timeout_resend);

+ client->attempt = 1;
+
switch (client->state) {

case DHCP_STATE_INIT:
@@ -268,6 +271,28 @@ static uint16_t client_checksum(void *buf, int len)
return ~((sum & 0xffff) + (sum >> 16));
}

+static void client_append_ip_headers(DHCPPacket *packet, uint16_t len)
+{
+ packet->ip.version = IPVERSION;
+ packet->ip.ihl = DHCP_IP_SIZE / 4;
+ packet->ip.tot_len = htons(len);
+
+ packet->ip.protocol = IPPROTO_UDP;
+ packet->ip.saddr = INADDR_ANY;
+ packet->ip.daddr = INADDR_BROADCAST;
+
+ packet->udp.source = htons(DHCP_PORT_CLIENT);
+ packet->udp.dest = htons(DHCP_PORT_SERVER);
+ packet->udp.len = htons(len - DHCP_IP_SIZE);
+
+ packet->ip.check = packet->udp.len;
+ packet->udp.check = client_checksum(&packet->ip.ttl, len - 8);
+
+ packet->ip.ttl = IPDEFTTL;
+ packet->ip.check = 0;
+ packet->ip.check = client_checksum(&packet->ip, DHCP_IP_SIZE);
+}
+
static int client_send_discover(DHCPClient *client, uint16_t secs)
{
int err = 0;
@@ -300,32 +325,61 @@ static int client_send_discover(DHCPClient *client, uint16_t secs)
if (err < 0)
goto error;

- discover->ip.version = IPVERSION;
- discover->ip.ihl = sizeof(discover->ip) >> 2;
- discover->ip.tot_len = htons(len);
+ client_append_ip_headers(discover, len);

- discover->ip.protocol = IPPROTO_UDP;
- discover->ip.saddr = INADDR_ANY;
- discover->ip.daddr = INADDR_BROADCAST;
+ err = __dhcp_network_send_raw_socket(client->fd, &client->link,
+ discover, len);
+
+error:
+ free(discover);

- discover->udp.source = htons(DHCP_PORT_CLIENT);
- discover->udp.dest = htons(DHCP_PORT_SERVER);
- discover->udp.len = htons(len - sizeof(discover->ip));
+ return err;
+}
+
+static int client_send_request(DHCPClient *client, uint16_t secs)
+{
+ DHCPPacket *request;
+ int optlen, len, err;
+ uint8_t *opt;
+
+ optlen = DHCP_CLIENT_MIN_OPTIONS_SIZE;
+ len = DHCP_MESSAGE_SIZE + optlen;
+
+ request = malloc0(len);
+
+ if (!request)
+ return -ENOBUFS;
+
+ err = client_packet_init(client, DHCP_REQUEST, &request->dhcp, secs,
+ &opt, &optlen);
+ if (err < 0)
+ goto error;
+
+ if (client->state == DHCP_STATE_REQUESTING) {
+ err = __dhcp_option_append(&opt, &optlen,
+ DHCP_OPTION_REQUESTED_IP_ADDRESS,
+ 4, &client->lease->address.s_addr);
+ if (err < 0)
+ goto error;
+
+ err = __dhcp_option_append(&opt, &optlen,
+ DHCP_OPTION_SERVER_IDENTIFIER,
+ 4, &client->lease->server_address.s_addr);
+ if (err < 0)
+ goto error;
+ }

- discover->ip.check = discover->udp.len;
- discover->udp.check = client_checksum(&discover->ip.ttl,
- len - 8);
+ err = __dhcp_option_append(&opt, &optlen, DHCP_OPTION_END, 0, NULL);
+ if (err < 0)
+ goto error;

- discover->ip.ttl = IPDEFTTL;
- discover->ip.check = 0;
- discover->ip.check = client_checksum(&discover->ip,
- sizeof(discover->ip));
+ client_append_ip_headers(request, len);

err = __dhcp_network_send_raw_socket(client->fd, &client->link,
- discover, len);
+ request, len);

error:
- free(discover);
+ free(request);

return err;
}
@@ -338,32 +392,50 @@ static int client_timeout_resend(sd_event_source *s, uint64_t usec,
uint16_t secs;
int err = 0;

+ secs = (usec - client->start_time) / USEC_PER_SEC;
+
+ if (client->attempt < 64)
+ client->attempt *= 2;
+
+ next_timeout = usec + (client->attempt - 1) * USEC_PER_SEC +
+ (random_u() & 0x1fffff);
+
+ err = sd_event_add_monotonic(sd_event_get(s), next_timeout,
+ 10 * USEC_PER_MSEC,
+ client_timeout_resend, client,
+ &client->timeout_resend);
+ if (err < 0)
+ goto error;
+
switch (client->state) {
case DHCP_STATE_INIT:
- case DHCP_STATE_SELECTING:
+ err = client_send_discover(client, secs);
+ if (err >= 0) {
+ client->state = DHCP_STATE_SELECTING;
+ client->attempt = 1;
+ } else {
+ if (client->attempt >= 64)
+ goto error;
+ }

- if (!client->start_time)
- client->start_time = usec;
+ break;

- secs = (usec - client->start_time) / USEC_PER_SEC;
+ case DHCP_STATE_SELECTING:
+ err = client_send_discover(client, secs);
+ if (err < 0 && client->attempt >= 64)
+ goto error;

- next_timeout = usec + 2 * USEC_PER_SEC + (random() & 0x1fffff);
+ break;

- err = sd_event_add_monotonic(sd_event_get(s), next_timeout,
- 10 * USEC_PER_MSEC,
- client_timeout_resend, client,
- &client->timeout_resend);
- if (err < 0)
+ case DHCP_STATE_REQUESTING:
+ err = client_send_request(client, secs);
+ if (err < 0 && client->attempt >= 64)
goto error;

- if (client_send_discover(client, secs) >= 0)
- client->state = DHCP_STATE_SELECTING;
-
break;

case DHCP_STATE_INIT_REBOOT:
case DHCP_STATE_REBOOTING:
- case DHCP_STATE_REQUESTING:
case DHCP_STATE_BOUND:
case DHCP_STATE_RENEWING:
case DHCP_STATE_REBINDING:
@@ -494,10 +566,8 @@ static int client_receive_raw_message(sd_event_source *s, int fd,
goto error;

len = read(fd, buf, buflen);
- if (len < 0) {
- err = -errno;
+ if (len < 0)
goto error;
- }

message = (DHCPPacket *)buf;

@@ -505,16 +575,19 @@ static int client_receive_raw_message(sd_event_source *s, int fd,
case DHCP_STATE_SELECTING:

if (client_receive_offer(client, message, len) >= 0) {
-
- close(client->fd);
- client->fd = 0;
- client->receive_message =
- sd_event_source_unref(client->receive_message);
-
client->timeout_resend =
sd_event_source_unref(client->timeout_resend);

client->state = DHCP_STATE_REQUESTING;
+ client->attempt = 1;
+
+ err = sd_event_add_monotonic(client->event,
+ now(CLOCK_MONOTONIC), 0,
+ client_timeout_resend,
+ client,
+ &client->timeout_resend);
+ if (err < 0)
+ goto error;
}

break;
@@ -535,6 +608,9 @@ static int client_receive_raw_message(sd_event_source *s, int fd,
return 0;

error:
+ if (err < 0)
+ return client_stop(client, err);
+
if (!err)
read(fd, buf, 1);

@@ -568,7 +644,8 @@ int dhcp_client_start(DHCPClient *client)
if (err < 0)
goto error;

- err = sd_event_add_monotonic(client->event, now(CLOCK_MONOTONIC), 0,
+ client->start_time = now(CLOCK_MONOTONIC);
+ err = sd_event_add_monotonic(client->event, client->start_time, 0,
client_timeout_resend, client,
&client->timeout_resend);
if (err < 0)
@@ -602,6 +679,7 @@ DHCPClient *dhcp_client_new(sd_event *event)
client->event = event;
client->state = DHCP_STATE_INIT;
client->index = -1;
+ client->attempt = 1;

client->req_opts_size = sizeof(default_req_opts)
/ sizeof(default_req_opts[0]);
--
1.7.10.4
Patrik Flykt
2013-11-13 21:22:54 UTC
Permalink
Define a notification callback and events for stopping and client
lease expiry. Add functions to fetch IP parameters from a lease.
---
src/dhcp/client.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
src/dhcp/client.h | 9 +++++
2 files changed, 106 insertions(+), 1 deletion(-)

diff --git a/src/dhcp/client.c b/src/dhcp/client.c
index 50008a9..0144c70 100644
--- a/src/dhcp/client.c
+++ b/src/dhcp/client.c
@@ -63,6 +63,8 @@ struct DHCPClient {
sd_event_source *timeout_expire;
sd_event_source *timeout_t1;
sd_event_source *timeout_t2;
+ dhcp_client_cb_t cb;
+ void *userdata;
DHCPLease *lease;
};

@@ -75,6 +77,19 @@ static uint8_t default_req_opts[] = {
DHCP_OPTION_NTP_SERVER,
};

+
+int dhcp_client_set_callback(DHCPClient *client, dhcp_client_cb_t cb,
+ void *userdata)
+{
+ if (!client)
+ return -EINVAL;
+
+ client->cb = cb;
+ client->userdata = userdata;
+
+ return 0;
+}
+
int dhcp_client_set_request_option(DHCPClient *client, uint8_t option)
{
int i;
@@ -157,8 +172,83 @@ int dhcp_client_set_mac(DHCPClient *client, struct ether_addr *addr)
return 0;
}

+int dhcp_client_get_address(DHCPClient *client, struct in_addr *addr)
+{
+ if (!addr || !client)
+ return -EINVAL;
+
+ switch (client->state) {
+ case DHCP_STATE_INIT:
+ case DHCP_STATE_SELECTING:
+ case DHCP_STATE_INIT_REBOOT:
+ case DHCP_STATE_REBOOTING:
+ case DHCP_STATE_REQUESTING:
+ return -EADDRNOTAVAIL;
+
+ case DHCP_STATE_BOUND:
+ case DHCP_STATE_RENEWING:
+ case DHCP_STATE_REBINDING:
+ addr->s_addr = client->lease->address.s_addr;
+
+ break;
+ }
+
+ return 0;
+}
+
+int dhcp_client_get_netmask(DHCPClient *client, struct in_addr *addr)
+{
+ if (!addr || !client)
+ return -EINVAL;
+
+ switch (client->state) {
+ case DHCP_STATE_INIT:
+ case DHCP_STATE_SELECTING:
+ case DHCP_STATE_INIT_REBOOT:
+ case DHCP_STATE_REBOOTING:
+ case DHCP_STATE_REQUESTING:
+ return -EADDRNOTAVAIL;
+
+ case DHCP_STATE_BOUND:
+ case DHCP_STATE_RENEWING:
+ case DHCP_STATE_REBINDING:
+ addr->s_addr = client->lease->subnet_mask.s_addr;
+
+ break;
+ }
+
+ return 0;
+}
+
+int dhcp_client_get_router(DHCPClient *client, struct in_addr *addr)
+{
+ if (!addr || !client)
+ return -EINVAL;
+
+ switch (client->state) {
+ case DHCP_STATE_INIT:
+ case DHCP_STATE_SELECTING:
+ case DHCP_STATE_INIT_REBOOT:
+ case DHCP_STATE_REBOOTING:
+ case DHCP_STATE_REQUESTING:
+ return -EADDRNOTAVAIL;
+
+ case DHCP_STATE_BOUND:
+ case DHCP_STATE_RENEWING:
+ case DHCP_STATE_REBINDING:
+ addr->s_addr = client->lease->router.s_addr;
+
+ break;
+ }
+
+ return 0;
+}
+
static int client_notify(DHCPClient *client, int event)
{
+ if (client->cb)
+ client->cb(client, event, client->userdata);
+
return 0;
}

@@ -195,6 +285,8 @@ static int client_stop(DHCPClient *client, int error)

client->attempt = 1;

+ client_notify(client, error);
+
switch (client->state) {

case DHCP_STATE_INIT:
@@ -487,6 +579,10 @@ error:
static int client_timeout_expire(sd_event_source *s, uint64_t usec,
void *userdata)
{
+ DHCPClient *client = userdata;
+
+ client_stop(client, DHCP_EVENT_EXPIRED);
+
return 0;
}

@@ -895,7 +991,7 @@ error:

int dhcp_client_stop(DHCPClient *client)
{
- return client_stop(client, 0);
+ return client_stop(client, DHCP_EVENT_STOP);
}

DHCPClient *dhcp_client_new(sd_event *event)
diff --git a/src/dhcp/client.h b/src/dhcp/client.h
index 5ce7fb4..adb4443 100644
--- a/src/dhcp/client.h
+++ b/src/dhcp/client.h
@@ -26,19 +26,28 @@

#include "sd-event.h"

+#define DHCP_EVENT_STOP 0
#define DHCP_EVENT_NAK 1
#define DHCP_EVENT_IP_ACQUIRE 2
#define DHCP_EVENT_IP_CHANGE 3
+#define DHCP_EVENT_EXPIRED 4

struct DHCPClient;
typedef struct DHCPClient DHCPClient;

+typedef void (*dhcp_client_cb_t)(DHCPClient *client, int event, void *userdata);
+int dhcp_client_set_callback(DHCPClient *client, dhcp_client_cb_t cb,
+ void *userdata);
int dhcp_client_set_request_option(DHCPClient *client, uint8_t option);
int dhcp_client_set_request_address(DHCPClient *client,
struct in_addr *last_address);
int dhcp_client_set_index(DHCPClient *client, int interface_index);
int dhcp_client_set_mac(DHCPClient *client, struct ether_addr *addr);

+int dhcp_client_get_address(DHCPClient *client, struct in_addr *addr);
+int dhcp_client_get_netmask(DHCPClient *client, struct in_addr *addr);
+int dhcp_client_get_router(DHCPClient *client, struct in_addr *addr);
+
int dhcp_client_start(DHCPClient *client);
int dhcp_client_stop(DHCPClient *client);
DHCPClient *dhcp_client_new(sd_event *event);
--
1.7.10.4
Patrik Flykt
2013-11-13 21:22:36 UTC
Permalink
Add a structure describing the DHCP file, sname and trailing options
fields. Create a messge holding these fields and call the internal
option parsing function.

In the test callback function verify that only regular options are
passed and figure out which part of the DHCP message is the one that
is being processed. As the test program knows the full contents of
the test options in the test structure, skip all non-regular fields
and verify that the option provided to the callback indeed is the
one expected. Check also if non-regular option fields are to be
ignored in the end of the option field as the callback is not called
again and the final check when the whole message has been processed
needs to be successful.

Add a boolean flag for pretty-printing, anticipate there will be a
nice option to toggle it in the future.
---
src/dhcp/test-dhcp-option.c | 258 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 258 insertions(+)

diff --git a/src/dhcp/test-dhcp-option.c b/src/dhcp/test-dhcp-option.c
index 9302fd6..a2b79ce 100644
--- a/src/dhcp/test-dhcp-option.c
+++ b/src/dhcp/test-dhcp-option.c
@@ -10,6 +10,68 @@
#include "protocol.h"
#include "internal.h"

+struct option_desc {
+ uint8_t sname[64];
+ int snamelen;
+ uint8_t file[128];
+ int filelen;
+ uint8_t options[128];
+ int len;
+ bool success;
+ int filepos;
+ int snamepos;
+ int pos;
+};
+
+static bool verbose = false;
+
+static struct option_desc option_tests[] = {
+ { {}, 0, {}, 0, { 42, 5, 65, 66, 67, 68, 69 }, 7, false, },
+ { {}, 0, {}, 0, { 42, 5, 65, 66, 67, 68, 69, 0, 0,
+ DHCP_OPTION_MESSAGE_TYPE, 1, DHCP_ACK }, 12, true, },
+ { {}, 0, {}, 0, { 8, 255, 70, 71, 72 }, 5, false, },
+ { {}, 0, {}, 0, { 0x35, 0x01, 0x05, 0x36, 0x04, 0x01, 0x00, 0xa8,
+ 0xc0, 0x33, 0x04, 0x00, 0x01, 0x51, 0x80, 0x01,
+ 0x04, 0xff, 0xff, 0xff, 0x00, 0x03, 0x04, 0xc0,
+ 0xa8, 0x00, 0x01, 0x06, 0x04, 0xc0, 0xa8, 0x00,
+ 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, },
+ 40, true, },
+ { {}, 0, {}, 0, { DHCP_OPTION_MESSAGE_TYPE, 1, DHCP_OFFER,
+ 42, 3, 0, 0, 0 }, 8, true, },
+ { {}, 0, {}, 0, { 42, 2, 1, 2, 44 }, 5, false, },
+
+ { {}, 0,
+ { 222, 3, 1, 2, 3, DHCP_OPTION_MESSAGE_TYPE, 1, DHCP_NAK }, 8,
+ { DHCP_OPTION_OVERLOAD, 1, DHCP_OVERLOAD_FILE }, 3, true, },
+
+ { { 1, 4, 1, 2, 3, 4, DHCP_OPTION_MESSAGE_TYPE, 1, DHCP_ACK }, 9,
+ { 222, 3, 1, 2, 3 }, 5,
+ { DHCP_OPTION_OVERLOAD, 1,
+ DHCP_OVERLOAD_FILE|DHCP_OVERLOAD_SNAME }, 3, true, },
+};
+
+static const char *dhcp_type(int type)
+{
+ switch(type) {
+ case DHCP_DISCOVER:
+ return "DHCPDISCOVER";
+ case DHCP_OFFER:
+ return "DHCPOFFER";
+ case DHCP_REQUEST:
+ return "DHCPREQUEST";
+ case DHCP_DECLINE:
+ return "DHCPDECLINE";
+ case DHCP_ACK:
+ return "DHCPACK";
+ case DHCP_NAK:
+ return "DHCPNAK";
+ case DHCP_RELEASE:
+ return "DHCPRELEASE";
+ default:
+ return "unknown";
+ }
+}
+
static void test_invalid_buffer_length(void)
{
DHCPMessage message;
@@ -43,10 +105,206 @@ static void test_cookie(void)
free(message);
}

+static DHCPMessage *create_message(uint8_t *options, uint16_t optlen,
+ uint8_t *file, uint8_t filelen,
+ uint8_t *sname, uint8_t snamelen)
+{
+ DHCPMessage *message;
+ int len = sizeof(DHCPMessage) + 4 + optlen;
+ uint8_t *opt;
+
+ message = malloc0(len);
+ opt = (uint8_t *)(message + 1);
+
+ opt[0] = 99;
+ opt[1] = 130;
+ opt[2] = 83;
+ opt[3] = 99;
+
+ if (options && optlen)
+ memcpy(&opt[4], options, optlen);
+
+ if (file && filelen <= 128)
+ memcpy(&message->file, file, filelen);
+
+ if (sname && snamelen <= 64)
+ memcpy(&message->sname, sname, snamelen);
+
+ return message;
+}
+
+static void test_ignore_opts(uint8_t *descoption, int *descpos, int *desclen)
+{
+ while (*descpos < *desclen) {
+ switch(descoption[*descpos]) {
+ case DHCP_OPTION_PAD:
+ *descpos += 1;
+ break;
+
+ case DHCP_OPTION_MESSAGE_TYPE:
+ case DHCP_OPTION_OVERLOAD:
+ *descpos += 3;
+ break;
+
+ default:
+ return;
+ }
+ }
+}
+
+static int test_options_cb(uint8_t code, uint8_t len, uint8_t *option,
+ void *user_data)
+{
+ struct option_desc *desc = user_data;
+ uint8_t *descoption = NULL;
+ int *desclen = NULL, *descpos = NULL;
+ uint8_t optcode = 0;
+ uint8_t optlen = 0;
+ uint8_t i;
+
+ assert((!desc && !code && !len) || desc);
+
+ if (!desc)
+ return -EINVAL;
+
+ assert(code != DHCP_OPTION_PAD);
+ assert(code != DHCP_OPTION_END);
+ assert(code != DHCP_OPTION_MESSAGE_TYPE);
+ assert(code != DHCP_OPTION_OVERLOAD);
+
+ while (desc->pos >= 0 || desc->filepos >= 0 || desc->snamepos >= 0) {
+
+ if (desc->pos >= 0) {
+ descoption = &desc->options[0];
+ desclen = &desc->len;
+ descpos = &desc->pos;
+ } else if (desc->filepos >= 0) {
+ descoption = &desc->file[0];
+ desclen = &desc->filelen;
+ descpos = &desc->filepos;
+ } else if (desc->snamepos >= 0) {
+ descoption = &desc->sname[0];
+ desclen = &desc->snamelen;
+ descpos = &desc->snamepos;
+ }
+
+ assert(descoption && desclen && descpos);
+
+ if (*desclen)
+ test_ignore_opts(descoption, descpos, desclen);
+
+ if (*descpos < *desclen)
+ break;
+
+ if (*descpos == *desclen)
+ *descpos = -1;
+ }
+
+ assert(*descpos != -1);
+
+ optcode = descoption[*descpos];
+ optlen = descoption[*descpos + 1];
+
+ if (verbose)
+ printf("DHCP code %2d(%2d) len %2d(%2d) ", code, optcode,
+ len, optlen);
+
+ assert(code == optcode);
+ assert(len == optlen);
+
+ for (i = 0; i < len; i++) {
+
+ if (verbose)
+ printf("0x%02x(0x%02x) ", option[i],
+ descoption[*descpos + 2 + i]);
+
+ assert(option[i] == descoption[*descpos + 2 + i]);
+ }
+
+ if (verbose)
+ printf("\n");
+
+ *descpos += optlen + 2;
+
+ test_ignore_opts(descoption, descpos, desclen);
+
+ if (desc->pos != -1 && desc->pos == desc->len)
+ desc->pos = -1;
+
+ if (desc->filepos != -1 && desc->filepos == desc->filelen)
+ desc->filepos = -1;
+
+ if (desc->snamepos != -1 && desc->snamepos == desc->snamelen)
+ desc->snamepos = -1;
+
+ return 0;
+}
+
+static void test_options(struct option_desc *desc)
+{
+ uint8_t *options = NULL;
+ uint8_t *file = NULL;
+ uint8_t *sname = NULL;
+ int optlen = 0;
+ int filelen = 0;
+ int snamelen = 0;
+ int buflen = 0;
+ DHCPMessage *message;
+ int res;
+
+ if (desc) {
+ file = &desc->file[0];
+ filelen = desc->filelen;
+ if (!filelen)
+ desc->filepos = -1;
+
+ sname = &desc->sname[0];
+ snamelen = desc->snamelen;
+ if (!snamelen)
+ desc->snamepos = -1;
+
+ options = &desc->options[0];
+ optlen = desc->len;
+ desc->pos = 0;
+ }
+ message = create_message(options, optlen, file, filelen,
+ sname, snamelen);
+
+ buflen = sizeof(DHCPMessage) + 4 + optlen;
+
+ if (!desc) {
+ assert((res = __dhcp_option_parse(message, buflen,
+ test_options_cb,
+ NULL)) == -ENOMSG);
+ } else if (desc->success) {
+ assert((res = __dhcp_option_parse(message, buflen,
+ test_options_cb,
+ desc)) >= 0);
+ assert(desc->pos == -1 && desc->filepos == -1 &&
+ desc->snamepos == -1);
+ } else
+ assert((res = __dhcp_option_parse(message, buflen,
+ test_options_cb,
+ desc)) < 0);
+
+
+ if (verbose)
+ printf("DHCP type %s\n", dhcp_type(res));
+
+ free(message);
+}
+
int main(int argc, char *argv[])
{
+ unsigned int i;
+
test_invalid_buffer_length();
test_cookie();

+ test_options(NULL);
+
+ for (i = 0; i < sizeof(option_tests)/sizeof(struct option_desc); i++)
+ test_options(&option_tests[i]);
+
return 0;
}
--
1.7.10.4
Zbigniew Jędrzejewski-Szmek
2013-11-14 02:08:59 UTC
Permalink
Post by Patrik Flykt
Add a structure describing the DHCP file, sname and trailing options
fields. Create a messge holding these fields and call the internal
option parsing function.
In the test callback function verify that only regular options are
passed and figure out which part of the DHCP message is the one that
is being processed. As the test program knows the full contents of
the test options in the test structure, skip all non-regular fields
and verify that the option provided to the callback indeed is the
one expected. Check also if non-regular option fields are to be
ignored in the end of the option field as the callback is not called
again and the final check when the whole message has been processed
needs to be successful.
Add a boolean flag for pretty-printing, anticipate there will be a
nice option to toggle it in the future.
---
src/dhcp/test-dhcp-option.c | 258 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 258 insertions(+)
diff --git a/src/dhcp/test-dhcp-option.c b/src/dhcp/test-dhcp-option.c
index 9302fd6..a2b79ce 100644
--- a/src/dhcp/test-dhcp-option.c
+++ b/src/dhcp/test-dhcp-option.c
@@ -10,6 +10,68 @@
#include "protocol.h"
#include "internal.h"
+struct option_desc {
+ uint8_t sname[64];
+ int snamelen;
+ uint8_t file[128];
+ int filelen;
+ uint8_t options[128];
+ int len;
+ bool success;
+ int filepos;
+ int snamepos;
+ int pos;
+};
+
+static bool verbose = false;
+
+static struct option_desc option_tests[] = {
+ { {}, 0, {}, 0, { 42, 5, 65, 66, 67, 68, 69 }, 7, false, },
+ { {}, 0, {}, 0, { 42, 5, 65, 66, 67, 68, 69, 0, 0,
+ DHCP_OPTION_MESSAGE_TYPE, 1, DHCP_ACK }, 12, true, },
+ { {}, 0, {}, 0, { 8, 255, 70, 71, 72 }, 5, false, },
+ { {}, 0, {}, 0, { 0x35, 0x01, 0x05, 0x36, 0x04, 0x01, 0x00, 0xa8,
+ 0xc0, 0x33, 0x04, 0x00, 0x01, 0x51, 0x80, 0x01,
+ 0x04, 0xff, 0xff, 0xff, 0x00, 0x03, 0x04, 0xc0,
+ 0xa8, 0x00, 0x01, 0x06, 0x04, 0xc0, 0xa8, 0x00,
+ 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, },
+ 40, true, },
+ { {}, 0, {}, 0, { DHCP_OPTION_MESSAGE_TYPE, 1, DHCP_OFFER,
+ 42, 3, 0, 0, 0 }, 8, true, },
+ { {}, 0, {}, 0, { 42, 2, 1, 2, 44 }, 5, false, },
+
+ { {}, 0,
+ { 222, 3, 1, 2, 3, DHCP_OPTION_MESSAGE_TYPE, 1, DHCP_NAK }, 8,
+ { DHCP_OPTION_OVERLOAD, 1, DHCP_OVERLOAD_FILE }, 3, true, },
+
+ { { 1, 4, 1, 2, 3, 4, DHCP_OPTION_MESSAGE_TYPE, 1, DHCP_ACK }, 9,
+ { 222, 3, 1, 2, 3 }, 5,
+ { DHCP_OPTION_OVERLOAD, 1,
+ DHCP_OVERLOAD_FILE|DHCP_OVERLOAD_SNAME }, 3, true, },
+};
+
+static const char *dhcp_type(int type)
+{
+ switch(type) {
+ return "DHCPDISCOVER";
+ return "DHCPOFFER";
+ return "DHCPREQUEST";
+ return "DHCPDECLINE";
+ return "DHCPACK";
+ return "DHCPNAK";
+ return "DHCPRELEASE";
+ return "unknown";
+ }
+}
+
static void test_invalid_buffer_length(void)
{
DHCPMessage message;
@@ -43,10 +105,206 @@ static void test_cookie(void)
free(message);
}
+static DHCPMessage *create_message(uint8_t *options, uint16_t optlen,
+ uint8_t *file, uint8_t filelen,
+ uint8_t *sname, uint8_t snamelen)
+{
+ DHCPMessage *message;
+ int len = sizeof(DHCPMessage) + 4 + optlen;
+ uint8_t *opt;
+
+ message = malloc0(len);
+ opt = (uint8_t *)(message + 1);
+
+ opt[0] = 99;
+ opt[1] = 130;
+ opt[2] = 83;
+ opt[3] = 99;
+
+ if (options && optlen)
+ memcpy(&opt[4], options, optlen);
+
+ if (file && filelen <= 128)
+ memcpy(&message->file, file, filelen);
+
+ if (sname && snamelen <= 64)
+ memcpy(&message->sname, sname, snamelen);
+
+ return message;
+}
+
+static void test_ignore_opts(uint8_t *descoption, int *descpos, int *desclen)
+{
+ while (*descpos < *desclen) {
+ switch(descoption[*descpos]) {
+ *descpos += 1;
+ break;
+
+ *descpos += 3;
+ break;
+
+ return;
+ }
+ }
+}
+
+static int test_options_cb(uint8_t code, uint8_t len, uint8_t *option,
+ void *user_data)
+{
+ struct option_desc *desc = user_data;
+ uint8_t *descoption = NULL;
+ int *desclen = NULL, *descpos = NULL;
+ uint8_t optcode = 0;
+ uint8_t optlen = 0;
+ uint8_t i;
+
+ assert((!desc && !code && !len) || desc);
+
+ if (!desc)
+ return -EINVAL;
+
+ assert(code != DHCP_OPTION_PAD);
+ assert(code != DHCP_OPTION_END);
+ assert(code != DHCP_OPTION_MESSAGE_TYPE);
+ assert(code != DHCP_OPTION_OVERLOAD);
+
+ while (desc->pos >= 0 || desc->filepos >= 0 || desc->snamepos >= 0) {
+
+ if (desc->pos >= 0) {
+ descoption = &desc->options[0];
+ desclen = &desc->len;
+ descpos = &desc->pos;
+ } else if (desc->filepos >= 0) {
+ descoption = &desc->file[0];
+ desclen = &desc->filelen;
+ descpos = &desc->filepos;
+ } else if (desc->snamepos >= 0) {
+ descoption = &desc->sname[0];
+ desclen = &desc->snamelen;
+ descpos = &desc->snamepos;
+ }
+
+ assert(descoption && desclen && descpos);
+
+ if (*desclen)
+ test_ignore_opts(descoption, descpos, desclen);
+
+ if (*descpos < *desclen)
+ break;
+
+ if (*descpos == *desclen)
+ *descpos = -1;
+ }
+
+ assert(*descpos != -1);
+
+ optcode = descoption[*descpos];
+ optlen = descoption[*descpos + 1];
+
+ if (verbose)
+ printf("DHCP code %2d(%2d) len %2d(%2d) ", code, optcode,
+ len, optlen);
+
+ assert(code == optcode);
+ assert(len == optlen);
+
+ for (i = 0; i < len; i++) {
+
+ if (verbose)
+ printf("0x%02x(0x%02x) ", option[i],
+ descoption[*descpos + 2 + i]);
+
+ assert(option[i] == descoption[*descpos + 2 + i]);
+ }
+
+ if (verbose)
+ printf("\n");
+
+ *descpos += optlen + 2;
+
+ test_ignore_opts(descoption, descpos, desclen);
+
+ if (desc->pos != -1 && desc->pos == desc->len)
+ desc->pos = -1;
+
+ if (desc->filepos != -1 && desc->filepos == desc->filelen)
+ desc->filepos = -1;
+
+ if (desc->snamepos != -1 && desc->snamepos == desc->snamelen)
+ desc->snamepos = -1;
+
+ return 0;
+}
+
+static void test_options(struct option_desc *desc)
+{
+ uint8_t *options = NULL;
+ uint8_t *file = NULL;
+ uint8_t *sname = NULL;
+ int optlen = 0;
+ int filelen = 0;
+ int snamelen = 0;
+ int buflen = 0;
+ DHCPMessage *message;
_cleanup_free_
Post by Patrik Flykt
+ int res;
+
+ if (desc) {
+ file = &desc->file[0];
+ filelen = desc->filelen;
+ if (!filelen)
+ desc->filepos = -1;
+
+ sname = &desc->sname[0];
+ snamelen = desc->snamelen;
+ if (!snamelen)
+ desc->snamepos = -1;
+
+ options = &desc->options[0];
+ optlen = desc->len;
+ desc->pos = 0;
+ }
+ message = create_message(options, optlen, file, filelen,
+ sname, snamelen);
+
+ buflen = sizeof(DHCPMessage) + 4 + optlen;
+
+ if (!desc) {
+ assert((res = __dhcp_option_parse(message, buflen,
+ test_options_cb,
+ NULL)) == -ENOMSG);
+ } else if (desc->success) {
+ assert((res = __dhcp_option_parse(message, buflen,
+ test_options_cb,
+ desc)) >= 0);
+ assert(desc->pos == -1 && desc->filepos == -1 &&
+ desc->snamepos == -1);
+ } else
+ assert((res = __dhcp_option_parse(message, buflen,
+ test_options_cb,
+ desc)) < 0);
+
+
+ if (verbose)
+ printf("DHCP type %s\n", dhcp_type(res));
+
+ free(message);
+}
+
int main(int argc, char *argv[])
{
+ unsigned int i;
+
test_invalid_buffer_length();
test_cookie();
+ test_options(NULL);
+
+ for (i = 0; i < sizeof(option_tests)/sizeof(struct option_desc); i++)
ELEMENTSOF
Post by Patrik Flykt
+ test_options(&option_tests[i]);
+
return 0;
}
Zbyszek
Patrik Flykt
2013-11-13 21:22:55 UTC
Permalink
---
src/dhcp/client.c | 11 +++++++++++
src/dhcp/client.h | 2 ++
2 files changed, 13 insertions(+)

diff --git a/src/dhcp/client.c b/src/dhcp/client.c
index 0144c70..a274549 100644
--- a/src/dhcp/client.c
+++ b/src/dhcp/client.c
@@ -994,6 +994,17 @@ int dhcp_client_stop(DHCPClient *client)
return client_stop(client, DHCP_EVENT_STOP);
}

+void dhcp_client_free(DHCPClient *client)
+{
+ if (!client)
+ return;
+
+ dhcp_client_stop(client);
+
+ free(client->req_opts);
+ free(client);
+}
+
DHCPClient *dhcp_client_new(sd_event *event)
{
DHCPClient *client;
diff --git a/src/dhcp/client.h b/src/dhcp/client.h
index adb4443..55fec93 100644
--- a/src/dhcp/client.h
+++ b/src/dhcp/client.h
@@ -50,4 +50,6 @@ int dhcp_client_get_router(DHCPClient *client, struct in_addr *addr);

int dhcp_client_start(DHCPClient *client);
int dhcp_client_stop(DHCPClient *client);
+
+void dhcp_client_free(DHCPClient *client);
DHCPClient *dhcp_client_new(sd_event *event);
--
1.7.10.4
Patrik Flykt
2013-11-13 21:22:46 UTC
Permalink
The client is stopped and brought back to its initial state.
---
src/dhcp/client.c | 35 +++++++++++++++++++++++++++++++++++
src/dhcp/client.h | 1 +
2 files changed, 36 insertions(+)

diff --git a/src/dhcp/client.c b/src/dhcp/client.c
index dc92880..79ac93b 100644
--- a/src/dhcp/client.c
+++ b/src/dhcp/client.c
@@ -133,6 +133,36 @@ int dhcp_client_set_mac(DHCPClient *client, struct ether_addr *addr)
return 0;
}

+static int client_stop(DHCPClient *client, int error)
+{
+ if (!client)
+ return -EINVAL;
+
+ if (client->state == DHCP_STATE_INIT ||
+ client->state == DHCP_STATE_INIT_REBOOT)
+ return -EALREADY;
+
+ switch (client->state) {
+
+ case DHCP_STATE_INIT:
+ case DHCP_STATE_SELECTING:
+
+ client->state = DHCP_STATE_INIT;
+ break;
+
+ case DHCP_STATE_INIT_REBOOT:
+ case DHCP_STATE_REBOOTING:
+ case DHCP_STATE_REQUESTING:
+ case DHCP_STATE_BOUND:
+ case DHCP_STATE_RENEWING:
+ case DHCP_STATE_REBINDING:
+
+ break;
+ }
+
+ return 0;
+}
+
static int client_packet_init(DHCPClient *client, uint8_t type,
DHCPMessage *message, uint16_t secs,
uint8_t **opt, int *optlen)
@@ -278,6 +308,11 @@ int dhcp_client_start(DHCPClient *client)
return client_send_discover(client, 0);
}

+int dhcp_client_stop(DHCPClient *client)
+{
+ return client_stop(client, 0);
+}
+
DHCPClient *dhcp_client_new(void)
{
DHCPClient *client;
diff --git a/src/dhcp/client.h b/src/dhcp/client.h
index fb7682a..71fdbe3 100644
--- a/src/dhcp/client.h
+++ b/src/dhcp/client.h
@@ -34,4 +34,5 @@ int dhcp_client_set_index(DHCPClient *client, int interface_index);
int dhcp_client_set_mac(DHCPClient *client, struct ether_addr *addr);

int dhcp_client_start(DHCPClient *client);
+int dhcp_client_stop(DHCPClient *client);
DHCPClient *dhcp_client_new(void);
--
1.7.10.4
Patrik Flykt
2013-11-13 21:22:49 UTC
Permalink
Create a function for handling the full IP, UDP and DHCP packet
and tie it to the main loop. Verify IP and UDP headers and checksum.
Creat a new lease structure with using the values supplied in the
DHCP message. Free the lease structure when client is stopped.

Split out socket handling into a creation and a sending part. As a
result modify the test code.
---
src/dhcp/client.c | 207 ++++++++++++++++++++++++++++++++++++++++++-
src/dhcp/internal.h | 4 +
src/dhcp/network.c | 33 +++----
src/dhcp/protocol.h | 6 ++
src/dhcp/test-dhcp-client.c | 21 ++++-
5 files changed, 253 insertions(+), 18 deletions(-)

diff --git a/src/dhcp/client.c b/src/dhcp/client.c
index cb3d41b..84d5c25 100644
--- a/src/dhcp/client.c
+++ b/src/dhcp/client.c
@@ -32,17 +32,31 @@

#define DHCP_CLIENT_MIN_OPTIONS_SIZE 312

+struct DHCPLease {
+ uint32_t lifetime;
+ struct in_addr address;
+ struct in_addr server_address;
+ struct in_addr subnet_mask;
+ struct in_addr router;
+};
+
+typedef struct DHCPLease DHCPLease;
+
struct DHCPClient {
DHCPState state;
sd_event *event;
sd_event_source *timeout_resend;
int index;
+ int fd;
+ struct sockaddr_ll link;
+ sd_event_source *receive_message;
uint8_t *req_opts;
int req_opts_size;
struct in_addr *last_addr;
struct ether_addr mac_addr;
uint32_t xid;
uint64_t start_time;
+ DHCPLease *lease;
};

static uint8_t default_req_opts[] = {
@@ -145,6 +159,14 @@ static int client_stop(DHCPClient *client, int error)
client->state == DHCP_STATE_INIT_REBOOT)
return -EALREADY;

+ if (client->fd > 0)
+ close(client->fd);
+ client->fd = 0;
+
+ if (client->receive_message)
+ client->receive_message =
+ sd_event_source_unref(client->receive_message);
+
if (client->timeout_resend)
client->timeout_resend =
sd_event_source_unref(client->timeout_resend);
@@ -154,6 +176,7 @@ static int client_stop(DHCPClient *client, int error)
case DHCP_STATE_INIT:
case DHCP_STATE_SELECTING:

+ client->start_time = 0;
client->state = DHCP_STATE_INIT;
break;

@@ -167,6 +190,11 @@ static int client_stop(DHCPClient *client, int error)
break;
}

+ if (client->lease) {
+ free(client->lease);
+ client->lease = NULL;
+ }
+
return 0;
}

@@ -293,7 +321,8 @@ static int client_send_discover(DHCPClient *client, uint16_t secs)
discover->ip.check = client_checksum(&discover->ip,
sizeof(discover->ip));

- err = __dhcp_network_send_raw_packet(client->index, discover, len);
+ err = __dhcp_network_send_raw_socket(client->fd, &client->link,
+ discover, len);

error:
free(discover);
@@ -350,6 +379,168 @@ error:
return 0;
}

+static int client_parse_offer(uint8_t code, uint8_t len, uint8_t *option,
+ void *user_data)
+{
+ DHCPLease *lease = user_data;
+ uint32_t val;
+
+ switch(code) {
+
+ case DHCP_OPTION_IP_ADDRESS_LEASE_TIME:
+ if (len == 4) {
+ memcpy(&val, option, 4);
+ lease->lifetime = ntohl(val);
+ }
+
+ break;
+
+ case DHCP_OPTION_SERVER_IDENTIFIER:
+ if (len >= 4)
+ memcpy(&lease->server_address.s_addr, option, 4);
+
+ break;
+
+ case DHCP_OPTION_SUBNET_MASK:
+ if (len >= 4)
+ memcpy(&lease->subnet_mask.s_addr, option, 4);
+
+ break;
+
+ case DHCP_OPTION_ROUTER:
+ if (len >= 4)
+ memcpy(&lease->router.s_addr, option, 4);
+
+ break;
+ }
+
+ return 0;
+}
+
+static int client_receive_offer(DHCPClient *client,
+ DHCPPacket *offer, int len)
+{
+ int hdrlen;
+ DHCPLease *lease;
+
+ if (len < (DHCP_IP_UDP_SIZE + DHCP_MESSAGE_SIZE))
+ return -EINVAL;
+
+ hdrlen = offer->ip.ihl * 4;
+ if (hdrlen < 20 || hdrlen > len || client_checksum(&offer->ip,
+ hdrlen))
+ return -EINVAL;
+
+ offer->ip.check = offer->udp.len;
+ offer->ip.ttl = 0;
+
+ if (hdrlen + ntohs(offer->udp.len) > len ||
+ client_checksum(&offer->ip.ttl, ntohs(offer->udp.len) + 12))
+ return -EINVAL;
+
+ if (ntohs(offer->udp.source) != DHCP_PORT_SERVER ||
+ ntohs(offer->udp.dest) != DHCP_PORT_CLIENT)
+ return -EINVAL;
+
+ if (offer->dhcp.op != BOOTREPLY)
+ return -EINVAL;
+
+ if (ntohl(offer->dhcp.xid) != client->xid)
+ return -EINVAL;
+
+ if (memcmp(&offer->dhcp.chaddr[0], &client->mac_addr.ether_addr_octet,
+ ETHER_ADDR_LEN))
+ return -EINVAL;
+
+ lease = new0(DHCPLease, 1);
+ if (!lease)
+ return -ENOBUFS;
+
+ len = len - DHCP_IP_UDP_SIZE;
+ if (__dhcp_option_parse(&offer->dhcp, len, client_parse_offer,
+ lease) != DHCP_OFFER)
+ goto error;
+
+ lease->address.s_addr = offer->dhcp.yiaddr;
+
+ if (lease->address.s_addr == INADDR_ANY ||
+ lease->server_address.s_addr == INADDR_ANY ||
+ lease->subnet_mask.s_addr == INADDR_ANY ||
+ lease->lifetime == 0)
+ goto error;
+
+ client->lease = lease;
+
+ return 0;
+
+error:
+ free(lease);
+
+ return -ENOMSG;
+}
+
+static int client_receive_raw_message(sd_event_source *s, int fd,
+ uint32_t revents, void *userdata)
+{
+ DHCPClient *client = userdata;
+ int err = 0;
+ int len, buflen;
+ uint8_t *buf;
+ DHCPPacket *message;
+
+ buflen = sizeof(DHCPPacket) + DHCP_CLIENT_MIN_OPTIONS_SIZE;
+ buf = malloc0(buflen);
+ if (!buf)
+ goto error;
+
+ len = read(fd, buf, buflen);
+ if (len < 0) {
+ err = -errno;
+ goto error;
+ }
+
+ message = (DHCPPacket *)buf;
+
+ switch (client->state) {
+ case DHCP_STATE_SELECTING:
+
+ if (client_receive_offer(client, message, len) >= 0) {
+
+ close(client->fd);
+ client->fd = 0;
+ client->receive_message =
+ sd_event_source_unref(client->receive_message);
+
+ client->timeout_resend =
+ sd_event_source_unref(client->timeout_resend);
+
+ client->state = DHCP_STATE_REQUESTING;
+ }
+
+ break;
+
+ case DHCP_STATE_INIT:
+ case DHCP_STATE_INIT_REBOOT:
+ case DHCP_STATE_REBOOTING:
+ case DHCP_STATE_REQUESTING:
+ case DHCP_STATE_BOUND:
+ case DHCP_STATE_RENEWING:
+ case DHCP_STATE_REBINDING:
+
+ break;
+ }
+
+ free(buf);
+
+ return 0;
+
+error:
+ if (!err)
+ read(fd, buf, 1);
+
+ return 0;
+}
+
int dhcp_client_start(DHCPClient *client)
{
int err;
@@ -363,6 +554,20 @@ int dhcp_client_start(DHCPClient *client)

client->xid = random_u();

+ client->fd = __dhcp_network_bind_raw_socket(client->index,
+ &client->link);
+
+ if (client->fd < 0) {
+ err = client->fd;
+ goto error;
+ }
+
+ err = sd_event_add_io(client->event, client->fd, EPOLLIN,
+ client_receive_raw_message, client,
+ &client->receive_message);
+ if (err < 0)
+ goto error;
+
err = sd_event_add_monotonic(client->event, now(CLOCK_MONOTONIC), 0,
client_timeout_resend, client,
&client->timeout_resend);
diff --git a/src/dhcp/internal.h b/src/dhcp/internal.h
index 92bd2a1..07bd677 100644
--- a/src/dhcp/internal.h
+++ b/src/dhcp/internal.h
@@ -22,9 +22,13 @@
***/

#include <stdint.h>
+#include <linux/if_packet.h>

#include "protocol.h"

+int __dhcp_network_bind_raw_socket(int index, struct sockaddr_ll *link);
+int __dhcp_network_send_raw_socket(int s, struct sockaddr_ll *link,
+ void *packet, int len);
int __dhcp_network_send_raw_packet(int index, void *packet, int len);

int __dhcp_option_append(uint8_t **buf, int *buflen, uint8_t code,
diff --git a/src/dhcp/network.c b/src/dhcp/network.c
index 623251d..1b25f86 100644
--- a/src/dhcp/network.c
+++ b/src/dhcp/network.c
@@ -28,36 +28,39 @@

#include "internal.h"

-int __dhcp_network_send_raw_packet(int index, void *packet, int len)
+int __dhcp_network_bind_raw_socket(int index, struct sockaddr_ll *link)
{
int err, s;
- struct sockaddr_ll link;
-
- err = 0;

s = socket(AF_PACKET, SOCK_DGRAM | SOCK_CLOEXEC, htons(ETH_P_IP));
if (s < 0)
return -errno;

- memset(&link, 0, sizeof(link));
+ memset(link, 0, sizeof(struct sockaddr_ll));

- link.sll_family = AF_PACKET;
- link.sll_protocol = htons(ETH_P_IP);
- link.sll_ifindex = index;
- link.sll_halen = ETH_ALEN;
- memset(&link.sll_addr, 0xff, ETH_ALEN);
+ link->sll_family = AF_PACKET;
+ link->sll_protocol = htons(ETH_P_IP);
+ link->sll_ifindex = index;
+ link->sll_halen = ETH_ALEN;
+ memset(link->sll_addr, 0xff, ETH_ALEN);

- if (bind(s, (struct sockaddr *)&link, sizeof(link)) < 0) {
+ if (bind(s, (struct sockaddr *)link, sizeof(struct sockaddr_ll)) < 0) {
err = -errno;
close(s);
return err;
}

- if (sendto(s, packet, len, 0, (struct sockaddr *)&link,
- sizeof(link)) < 0)
- err = -errno;
+ return s;
+}

- close(s);
+int __dhcp_network_send_raw_socket(int s, struct sockaddr_ll *link,
+ void *packet, int len)
+{
+ int err = 0;
+
+ if (sendto(s, packet, len, 0, (struct sockaddr *)link,
+ sizeof(struct sockaddr_ll)) < 0)
+ err = -errno;

return err;
}
diff --git a/src/dhcp/protocol.h b/src/dhcp/protocol.h
index a2c5216..014a6f8 100644
--- a/src/dhcp/protocol.h
+++ b/src/dhcp/protocol.h
@@ -52,6 +52,10 @@ struct DHCPPacket {

typedef struct DHCPPacket DHCPPacket;

+#define DHCP_IP_SIZE (int32_t)(sizeof(struct iphdr))
+#define DHCP_IP_UDP_SIZE (int32_t)(sizeof(struct udphdr) + DHCP_IP_SIZE)
+#define DHCP_MESSAGE_SIZE (int32_t)(sizeof(DHCPMessage))
+
#define DHCP_PORT_SERVER 67
#define DHCP_PORT_CLIENT 68

@@ -90,8 +94,10 @@ typedef enum DHCPState DHCPState;
#define DHCP_OPTION_DOMAIN_NAME 15
#define DHCP_OPTION_NTP_SERVER 42
#define DHCP_OPTION_REQUESTED_IP_ADDRESS 50
+#define DHCP_OPTION_IP_ADDRESS_LEASE_TIME 51
#define DHCP_OPTION_OVERLOAD 52
#define DHCP_OPTION_MESSAGE_TYPE 53
+#define DHCP_OPTION_SERVER_IDENTIFIER 54
#define DHCP_OPTION_PARAMETER_REQUEST_LIST 55
#define DHCP_OPTION_CLIENT_IDENTIFIER 61
#define DHCP_OPTION_END 255
diff --git a/src/dhcp/test-dhcp-client.c b/src/dhcp/test-dhcp-client.c
index 9cad04b..dc2420b 100644
--- a/src/dhcp/test-dhcp-client.c
+++ b/src/dhcp/test-dhcp-client.c
@@ -24,6 +24,9 @@
#include <errno.h>
#include <stdio.h>
#include <string.h>
+#include <sys/types.h>
+#include <sys/socket.h>
+#include <unistd.h>

#include "protocol.h"
#include "internal.h"
@@ -33,6 +36,8 @@ static struct ether_addr mac_addr = {
.ether_addr_octet = {'A', 'B', 'C', '1', '2', '3'}
};

+static int test_fd[2];
+
static void test_request_basic(sd_event *e)
{
DHCPClient *client;
@@ -124,14 +129,15 @@ static int check_options(uint8_t code, uint8_t len, uint8_t *option,
return 0;
}

-int __dhcp_network_send_raw_packet(int index, void *packet, int len)
+int __dhcp_network_send_raw_socket(int s, struct sockaddr_ll *link,
+ void *packet, int len)
{
int size;
DHCPPacket *discover;
uint16_t ip_check, udp_check;
int res;

- assert(index == 42);
+ assert(s > 0);
assert(packet);

size = sizeof(DHCPPacket) + 4;
@@ -171,6 +177,14 @@ int __dhcp_network_send_raw_packet(int index, void *packet, int len)
return 575;
}

+int __dhcp_network_bind_raw_socket(int index, struct sockaddr_ll *link)
+{
+ if (socketpair(AF_UNIX, SOCK_STREAM, 0, test_fd) < 0)
+ return -errno;
+
+ return test_fd[0];
+}
+
static void test_discover_message(sd_event *e)
{
DHCPClient *client;
@@ -187,6 +201,9 @@ static void test_discover_message(sd_event *e)
res = dhcp_client_start(client);

assert(res == 575 || res == 0);
+
+ close(test_fd[0]);
+ close(test_fd[1]);
}

int main(int argc, char *argv[])
--
1.7.10.4
Lennart Poettering
2013-11-14 00:38:12 UTC
Permalink
Post by Patrik Flykt
+ if (client->fd > 0)
+ close(client->fd);
+ client->fd = 0;
fd 0 is a valid fd (actually refers to stdin), I am pretty sure you want
to assign -1 here, no?
Post by Patrik Flykt
+ if (hdrlen + ntohs(offer->udp.len) > len ||
+ client_checksum(&offer->ip.ttl, ntohs(offer->udp.len) + 12))
+ return -EINVAL;
Hmm, so it might make sense to use betoh16() and friends instead of
ntohs() here, where it makes sense, and to declare the integers as
be16_t, be32_t and so on. This is defined in "sparse-endian.h" and is
useful for using the kernel sparse checker on this code, and it will
then detect whenever a non-native endian integer is accessed without
conversion.
Post by Patrik Flykt
+ lease = new0(DHCPLease, 1);
+ if (!lease)
+ return -ENOBUFS;
ENOMEM please
Post by Patrik Flykt
+
+ free(lease);
+
+ return -ENOMSG;
+}
_cleanup_free_!
Post by Patrik Flykt
+
+static int client_receive_raw_message(sd_event_source *s, int fd,
+ uint32_t revents, void *userdata)
+{
+ DHCPClient *client = userdata;
+ int err = 0;
+ int len, buflen;
+ uint8_t *buf;
+ DHCPPacket *message;
+
+ buflen = sizeof(DHCPPacket) + DHCP_CLIENT_MIN_OPTIONS_SIZE;
+ buf = malloc0(buflen);
+ if (!buf)
+ goto error;
Hmm, you eat up the rror here, is that intended?
Post by Patrik Flykt
+ len = read(fd, buf, buflen);
+ if (len < 0) {
+ err = -errno;
+ goto error;
+ }
+
+ message = (DHCPPacket *)buf;
+
+ switch (client->state) {
+
+ if (client_receive_offer(client, message, len) >= 0) {
+
+ close(client->fd);
+ client->fd = 0;
-1 again...
Post by Patrik Flykt
+ if (!err)
+ read(fd, buf, 1);
You forget to free buf here...

Lennart
--
Lennart Poettering, Red Hat
Patrik Flykt
2013-11-13 21:22:41 UTC
Permalink
---
Makefile.am | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)

diff --git a/Makefile.am b/Makefile.am
index aeca484..5c350ab 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -3740,6 +3740,26 @@ endif

# ------------------------------------------------------------------------------
if ENABLE_DHCP
+libsystemd_dhcp_la_SOURCES = \
+ src/dhcp/protocol.h \
+ src/dhcp/internal.h \
+ src/dhcp/network.c \
+ src/dhcp/option.c \
+ src/dhcp/client.h \
+ src/dhcp/client.c
+
+noinst_LTLIBRARIES += \
+ libsystemd-dhcp.la
+
+libsystemd_dhcp_la_CFLAGS = \
+ $(AM_CFLAGS)
+
+libsystemd_dhcp_la_LDFLAGS = \
+ $(AM_LDFLAGS)
+
+libsystemd_dhcp_la_LIBADD = \
+ libsystemd-shared.la
+
test_dhcp_option_SOURCES = \
src/dhcp/protocol.h \
src/dhcp/internal.h \
--
1.7.10.4
Lennart Poettering
2013-11-14 00:18:58 UTC
Permalink
Post by Patrik Flykt
---
Makefile.am | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/Makefile.am b/Makefile.am
index aeca484..5c350ab 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -3740,6 +3740,26 @@ endif
# ------------------------------------------------------------------------------
if ENABLE_DHCP
+libsystemd_dhcp_la_SOURCES = \
+ src/dhcp/protocol.h \
+ src/dhcp/internal.h \
+ src/dhcp/network.c \
+ src/dhcp/option.c \
+ src/dhcp/client.h \
+ src/dhcp/client.c
Which one of these should be public APIs? Please keep them minimal and
move them to src/systemd/ instead, and use the "sd-" prefix for them

Lennart
--
Lennart Poettering, Red Hat
Patrik Flykt
2013-11-13 21:22:45 UTC
Permalink
It was noticed by Grant Erickson in ConnMan commit
95e15c09350acf58d4707056ae2614570883ef66 that:

"Certain DHCP servers, such as that implemented in Mac OS X
(< 10.7) for its "Internet Sharing" feature, refuse to issue
a DHCP lease to clients that have not set a non-zero value
in their DISCOVER or REQUEST packets."
---
src/dhcp/client.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/src/dhcp/client.c b/src/dhcp/client.c
index 1d4d957..dc92880 100644
--- a/src/dhcp/client.c
+++ b/src/dhcp/client.c
@@ -134,8 +134,8 @@ int dhcp_client_set_mac(DHCPClient *client, struct ether_addr *addr)
}

static int client_packet_init(DHCPClient *client, uint8_t type,
- DHCPMessage *message, uint8_t **opt,
- int *optlen)
+ DHCPMessage *message, uint16_t secs,
+ uint8_t **opt, int *optlen)
{
int err;

@@ -149,6 +149,7 @@ static int client_packet_init(DHCPClient *client, uint8_t type,
message->htype = 1;
message->hlen = ETHER_ADDR_LEN;
message->xid = htonl(client->xid);
+ message->secs = htons(secs);

memcpy(&message->chaddr, &client->mac_addr, ETH_ALEN);
(*opt)[0] = 0x63;
@@ -202,7 +203,7 @@ static uint16_t client_checksum(void *buf, int len)
return ~((sum & 0xffff) + (sum >> 16));
}

-static int client_send_discover(DHCPClient *client)
+static int client_send_discover(DHCPClient *client, uint16_t secs)
{
int err = 0;
DHCPPacket *discover;
@@ -218,7 +219,7 @@ static int client_send_discover(DHCPClient *client)
return -ENOBUFS;

err = client_packet_init(client, DHCP_DISCOVER, &discover->dhcp,
- &opt, &optlen);
+ secs, &opt, &optlen);
if (err < 0)
goto error;

@@ -274,7 +275,7 @@ int dhcp_client_start(DHCPClient *client)

client->xid = random_u();

- return client_send_discover(client);
+ return client_send_discover(client, 0);
}

DHCPClient *dhcp_client_new(void)
--
1.7.10.4
Lennart Poettering
2013-11-14 00:23:54 UTC
Permalink
Post by Patrik Flykt
It was noticed by Grant Erickson in ConnMan commit
"Certain DHCP servers, such as that implemented in Mac OS X
(< 10.7) for its "Internet Sharing" feature, refuse to issue
a DHCP lease to clients that have not set a non-zero value
in their DISCOVER or REQUEST packets."
I am quite sure it would be better to add this comment to the sources
too, there are easier to find there, and people will less likely drop
the assignment when they revisit the code.

Lennart
--
Lennart Poettering, Red Hat
Patrik Flykt
2013-11-15 09:26:08 UTC
Permalink
Post by Lennart Poettering
Post by Patrik Flykt
It was noticed by Grant Erickson in ConnMan commit
"Certain DHCP servers, such as that implemented in Mac OS X
(< 10.7) for its "Internet Sharing" feature, refuse to issue
a DHCP lease to clients that have not set a non-zero value
in their DISCOVER or REQUEST packets."
I am quite sure it would be better to add this comment to the sources
too, there are easier to find there, and people will less likely drop
the assignment when they revisit the code.
RFC 2131 is a bit vague but indicates 'secs' field is a SHOULD for a
client. With that in mind I'd hope people would read the commit message
before removing code from a protocol, but I'll add a comment to make it
clear.

Cheers,

Patrik
Greg KH
2013-11-13 21:31:08 UTC
Permalink
Hi,
This patch set implements a DHCPv4 client library named libsystemd-dhcp
to be used with systemd-network.
Nice stuff. Where did this code come from, conman? If so, for some
reason I thought that it would be easier to make a library of the
existing conman dhcp code and have both projects use it instead of
forking and having two copies?

thanks,

greg k-h
Marcel Holtmann
2013-11-13 21:49:41 UTC
Permalink
Hi Greg,
Post by Greg KH
This patch set implements a DHCPv4 client library named libsystemd-dhcp
to be used with systemd-network.
Nice stuff. Where did this code come from, conman? If so, for some
reason I thought that it would be easier to make a library of the
existing conman dhcp code and have both projects use it instead of
forking and having two copies?
that is the long term plan. Once ConnMan is switching over to use libsystemd-bus and kdbus, we are switching over to using the systemd event loop instead of GLib main loop and then it will also start using libsystemd-dhcp.

Regards

Marcel
Colin Walters
2013-11-13 22:18:26 UTC
Permalink
Post by Marcel Holtmann
that is the long term plan. Once ConnMan is switching over to use libsystemd-bus and kdbus,
we are switching over to using the systemd event loop instead of GLib main loop
But I think the long term architecturally correct place for the core
system main loop is glibc, not systemd.

For example, that would allow runtimes like OpenJDK and Python to
unconditionally depend on it (if present).

As systemd's event loop becomes public API, it has the potential to
create interoperability problems for GNOME - think applications like
https://git.gnome.org/browse/gnome-logs
that want to both monitor the systemd journal and talk to GTK+.

At the moment it's OK because sd_journal has APIs sufficient to plug in
external loops.

Adding a mainloop API to glibc would be a lot of work - it might even
entail trying to get it by POSIX. At least it'd entail describing the
interaction with the other POSIX APIs. But I think that'd be worth the
effort.
Marcel Holtmann
2013-11-13 22:25:44 UTC
Permalink
Hi Colin,
Post by Colin Walters
Post by Marcel Holtmann
that is the long term plan. Once ConnMan is switching over to use libsystemd-bus and kdbus,
we are switching over to using the systemd event loop instead of GLib main loop
But I think the long term architecturally correct place for the core
system main loop is glibc, not systemd.
For example, that would allow runtimes like OpenJDK and Python to
unconditionally depend on it (if present).
As systemd's event loop becomes public API, it has the potential to
create interoperability problems for GNOME - think applications like
https://git.gnome.org/browse/gnome-logs
that want to both monitor the systemd journal and talk to GTK+.
At the moment it's OK because sd_journal has APIs sufficient to plug in
external loops.
Adding a mainloop API to glibc would be a lot of work - it might even
entail trying to get it by POSIX. At least it'd entail describing the
interaction with the other POSIX APIs. But I think that'd be worth the
effort.
I am a bit lost on your concerns here. Our focus for ConnMan is libsystemd-bus and kdbus support. Over time we will be removing our dependency on GLib completely. So the systemd event loop will become our only event loop.

And as a system daemon, I only care about writing to the Journal. No idea why I would need a event loop for that.

Regards

Marcel
Colin Walters
2013-11-13 22:51:30 UTC
Permalink
Post by Marcel Holtmann
I am a bit lost on your concerns here. Our focus for ConnMan is libsystemd-bus and kdbus support.
Yeah, sorry; I just kind of used your mail as a basis for the larger
picture of sd_event as public API.
Post by Marcel Holtmann
And as a system daemon, I only care about writing to the Journal.
No idea why I would need a event loop for that.
Ah...even if you're not spawning external processes, I'd imagine you at
least want g_timeout_add()/sd_event_add_monotonic() type thing where
library code manipulates the poll timeout.
Lennart Poettering
2013-11-13 23:47:24 UTC
Permalink
Post by Colin Walters
Post by Marcel Holtmann
that is the long term plan. Once ConnMan is switching over to use libsystemd-bus and kdbus,
we are switching over to using the systemd event loop instead of GLib main loop
But I think the long term architecturally correct place for the core
system main loop is glibc, not systemd.
For example, that would allow runtimes like OpenJDK and Python to
unconditionally depend on it (if present).
As systemd's event loop becomes public API, it has the potential to
create interoperability problems for GNOME - think applications like
https://git.gnome.org/browse/gnome-logs
that want to both monitor the systemd journal and talk to GTK+.
At the moment it's OK because sd_journal has APIs sufficient to plug in
external loops.
Adding a mainloop API to glibc would be a lot of work - it might even
entail trying to get it by POSIX. At least it'd entail describing the
interaction with the other POSIX APIs. But I think that'd be worth the
effort.
I am pretty sure it makes sense to have domain-specific event loops. I
am not convinced that it would even be possible to unify all event loop
implementations into one. For example, GLib and and sd-event support
priorization of events, which means we need to flush all queued events
from the kernel before we decide which one to run first. This however is
probably not the best choice for a number of other usecases, where you
want to optimize for ridiculous numbers of fds. Hence, I believe
developers should get a lot of freedom on how much or how little event
loop they want, and which implementation they choose.

Then, GMainLoop can do some things sd-event cannot do. sd-event can do a
number of things GMainLoop cannot do.

I doubt moving OpenJDK or Python to sd-event or something like it ever
would make sense. (Do either even have any event loop? Java at least
prefers async stuff via threads as the solution for IO, not multiplexing
via poll()-like stuff -- which they only added very very late to the
language...).

For all event loop consuming library code I write I will make sure that
it can be connected to any reasonable event loop, not only sd-event.

I mean, libsystemd-bus already includes two event loop implementations,
one in sd-event, and another trivial one in sd_bus_wait() in a way...

So, please do not understand sd-event as an attempt to get everybody to
use the same event loop. It's just an attempt to provide a good event
loop by default, that is more useful than pure epoll (such as sanely
handles lots of timers), that tries to provide some of the features that
GMainLoop supported since a longer time (like timer coalescing or
priorisation), that we need for low-level stuff (like OOM safety, this
stuff should run in PID1...), and is generally just code that people can
use, if they want, but don't have to. Not an attempt for standardisation
but an attempt to just provide code that solves naked epoll()'s problems,
without going overboard and turning into libev/libevent.

Lennart
--
Lennart Poettering, Red Hat
Zbigniew Jędrzejewski-Szmek
2013-11-13 23:56:36 UTC
Permalink
Post by Lennart Poettering
I doubt moving OpenJDK or Python to sd-event or something like it ever
would make sense. (Do either even have any event loop?
http://www.python.org/dev/peps/pep-3156/

Zbyszek
Colin Walters
2013-11-14 00:48:29 UTC
Permalink
Post by Lennart Poettering
I am pretty sure it makes sense to have domain-specific event loops. I
am not convinced that it would even be possible to unify all event loop
implementations into one. For example, GLib and and sd-event support
priorization of events,
Right, this is:
https://bugzilla.gnome.org/show_bug.cgi?id=156048
Post by Lennart Poettering
I doubt moving OpenJDK or Python to sd-event or something like it ever
would make sense. (Do either even have any event loop? Java at least
prefers async stuff via threads as the solution for IO, not multiplexing
via poll()-like stuff -- which they only added very very late to the
language...).
Yes, Java added it late, but it is AFAIK fairly actively used in some
subsets of that world. Zbigniew already linked to the Python version.
Post by Lennart Poettering
For all event loop consuming library code I write I will make sure that
it can be connected to any reasonable event loop, not only sd-event.
Ok, that's a reasonable short term answer.
Patrik Flykt
2013-11-13 22:11:44 UTC
Permalink
Hi,
Post by Greg KH
Nice stuff. Where did this code come from, conman? If so, for some
reason I thought that it would be easier to make a library of the
existing conman dhcp code and have both projects use it instead of
forking and having two copies?
Looking more carefully at the code in ConnMan started to reveal the
relative age of the DHCP client part and showed some interesting code
that might not be entirely correct. After removing the GLib main loop
parts not too much valuable code was left behind. I have kept one eye in
the existing code to find additions for picky servers so I hope the
lessons learned have not been lost. I'm more positive with the DHCPv6
part, that implementation has more reusable parts.

I'm not too worried about having two copies for now, the one in ConnMan
would have met its fate anyway a bit later.


Cheers,

Patrik
Zbigniew Jędrzejewski-Szmek
2013-11-13 23:43:59 UTC
Permalink
Post by Patrik Flykt
Provide functionality for initializing a DHCP client struct, setting
interface index, last used address and additional options to request.
On initialization the most useful options are added by default.
---
src/dhcp/client.c | 137 +++++++++++++++++++++++++++++++++++++++++++++++++++++
src/dhcp/client.h | 34 +++++++++++++
2 files changed, 171 insertions(+)
create mode 100644 src/dhcp/client.c
create mode 100644 src/dhcp/client.h
diff --git a/src/dhcp/client.c b/src/dhcp/client.c
new file mode 100644
index 0000000..172ddd9
--- /dev/null
+++ b/src/dhcp/client.c
@@ -0,0 +1,137 @@
+/***
+ This file is part of systemd.
+
+ Copyright (C) 2013 Intel Corporation. All rights reserved.
+
+ systemd is free software; you can redistribute it and/or modify it
+ under the terms of the GNU Lesser General Public License as published by
+ the Free Software Foundation; either version 2.1 of the License, or
+ (at your option) any later version.
+
+ systemd is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public License
+ along with systemd; If not, see <http://www.gnu.org/licenses/>.
+***/
+
+#include <stdlib.h>
+#include <errno.h>
+#include <string.h>
+#include <stdio.h>
+
+#include "util.h"
+#include "list.h"
+
+#include "protocol.h"
+#include "client.h"
+
+struct DHCPClient {
+ DHCPState state;
+ int index;
+ uint8_t *req_opts;
+ int req_opts_size;
+ struct in_addr *last_addr;
+};
+
+static uint8_t default_req_opts[] = {
+ DHCP_OPTION_SUBNET_MASK,
+ DHCP_OPTION_ROUTER,
+ DHCP_OPTION_HOST_NAME,
+ DHCP_OPTION_DOMAIN_NAME,
+ DHCP_OPTION_DOMAIN_NAME_SERVER,
+ DHCP_OPTION_NTP_SERVER,
+};
+
+int dhcp_client_set_request_option(DHCPClient *client, uint8_t option)
+{
+ int i;
+
+ if (!client)
+ return -EINVAL;
+
+ if (client->state != DHCP_STATE_INIT)
+ return -EBUSY;
+
+ switch(option) {
+ return -EINVAL;
+
+ break;
+ }
+
+ for (i = 0; i < client->req_opts_size; i++)
+ if (client->req_opts[i] == option)
+ return -EEXIST;
+
+ client->req_opts_size++;
+ client->req_opts = realloc(client->req_opts, client->req_opts_size);
+ if (!client->req_opts) {
+ client->req_opts_size = 0;
+ return -ENOBUFS;
+ }
Hm, you have memory leak here. And repeated realloc is slow.

Try something like this:
if (!GREEDY_REALLOC(client->req_opts,
client->req_opts_size,
client->req_opts_size + 1))
return -ENOMEM;

In general we use -ENOMEM, so for consistency it should be used here too.
Post by Patrik Flykt
+ client->req_opts[client->req_opts_size - 1] = option;
+
+ return 0;
+}
+
+int dhcp_client_set_request_address(DHCPClient *client,
+ struct in_addr *last_addr)
+{
+ if (!client)
+ return -EINVAL;
+
+ if (client->state != DHCP_STATE_INIT)
+ return -EBUSY;
+
+ client->last_addr = malloc(sizeof(struct in_addr));
+ if (!client->last_addr)
+ return -ENOBUFS;
+
+ memcpy(&client->last_addr, last_addr, sizeof(struct in_addr));
client->last_addr = memdup(last_addr, sizeof(struct in_addr));
Post by Patrik Flykt
+
+ return 0;
+}
+
+int dhcp_client_set_index(DHCPClient *client, int interface_index)
+{
+ if (!client || interface_index < -1)
+ return -EINVAL;
+
+ if (client->state != DHCP_STATE_INIT)
+ return -EBUSY;
+
+ client->index = interface_index;
+
+ return 0;
+}
+
+DHCPClient *dhcp_client_new(void)
+{
+ DHCPClient *client;
+ int i;
+
+ client = new0(DHCPClient, 1);
+ if (!client)
+ return NULL;
+
+ client->state = DHCP_STATE_INIT;
+ client->index = -1;
+
+ client->req_opts_size = sizeof(default_req_opts)
+ / sizeof(default_req_opts[0]);
ELEMENTSOF(default_req_opts)
Post by Patrik Flykt
+ client->req_opts = malloc(client->req_opts_size);
+ for (i = 0; i < client->req_opts_size; i++)
+ client->req_opts[i] = default_req_opts[i];
client->req_opts = memdup_multiply(client->req_opts_size, sizeof(default_req_opts),
client->req_opts_size);
Post by Patrik Flykt
+
+ return client;
+}
diff --git a/src/dhcp/client.h b/src/dhcp/client.h
new file mode 100644
index 0000000..447c03b
--- /dev/null
+++ b/src/dhcp/client.h
@@ -0,0 +1,34 @@
+/*-*- Mode: C; c-basic-offset: 8; indent-tabs-mode: nil -*-*/
+
+#pragma once
+
+/***
+ This file is part of systemd.
+
+ Copyright (C) 2013 Intel Corporation. All rights reserved.
+
+ systemd is free software; you can redistribute it and/or modify it
+ under the terms of the GNU Lesser General Public License as published by
+ the Free Software Foundation; either version 2.1 of the License, or
+ (at your option) any later version.
+
+ systemd is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public License
+ along with systemd; If not, see <http://www.gnu.org/licenses/>.
+***/
+
+#include <netinet/in.h>
+
+struct DHCPClient;
+typedef struct DHCPClient DHCPClient;
+
+int dhcp_client_set_request_option(DHCPClient *client, uint8_t option);
+int dhcp_client_set_request_address(DHCPClient *client,
+ struct in_addr *last_address);
+int dhcp_client_set_index(DHCPClient *client, int interface_index);
+
+DHCPClient *dhcp_client_new(void);
Zbyszek
Lennart Poettering
2013-11-14 00:01:47 UTC
Permalink
Post by Patrik Flykt
+static uint8_t default_req_opts[] = {
+ DHCP_OPTION_SUBNET_MASK,
+ DHCP_OPTION_ROUTER,
+ DHCP_OPTION_HOST_NAME,
+ DHCP_OPTION_DOMAIN_NAME,
+ DHCP_OPTION_DOMAIN_NAME_SERVER,
+ DHCP_OPTION_NTP_SERVER,
+};
Shouldn't this array be const?
Post by Patrik Flykt
+
+int dhcp_client_set_request_option(DHCPClient *client, uint8_t option)
+{
+ int i;
+
+ if (!client)
+ return -EINVAL;
Using assert_return() for this makes these simpler and more redable (and
also uses the _unlikely_() stuff). Use assert_return() for all those
cases where the caller made an obvious programming mistake.

Actually, to give some insight on the way how we did things so far in systemd:

- If something is internal code, then it uses assert() for checking for
programming errors
- If something is public API code, then it uses assert_return() for
checking for programming errors. Public APIs have the "sd_" prefix in
their API calls.

Basically, we are more forgiving towards public users of our code than
to ourselves.

Of course, please never use assert() nor assert_return() for checking
for runtime errors as opposed to programming errors.
Post by Patrik Flykt
+ client->req_opts_size++;
+ client->req_opts = realloc(client->req_opts, client->req_opts_size);
+ if (!client->req_opts) {
+ client->req_opts_size = 0;
+ return -ENOBUFS;
+ }
GREEDY_REALLOC() is cool.
Post by Patrik Flykt
+
+ client->req_opts[client->req_opts_size - 1] = option;
+
+ return 0;
+}
+
+int dhcp_client_set_request_address(DHCPClient *client,
+ struct in_addr *last_addr)
The parameter should be const, no?
Post by Patrik Flykt
+{
+ if (!client)
+ return -EINVAL;
+
+ if (client->state != DHCP_STATE_INIT)
+ return -EBUSY;
+
+ client->last_addr = malloc(sizeof(struct in_addr));
+ if (!client->last_addr)
+ return -ENOBUFS;
Hmm, "struct in_addr" contains nothing but a uint32_t. It really sounds
overkill allocating such a structure individually on the
stack. Shouldn't this structure be directly inside DHCPClient? Or
actually, do we really want to use struct in_addr at all? It sounds so
useless, a "uint_32_t" sounds so much simpler, and easier to use...
Post by Patrik Flykt
+
+ memcpy(&client->last_addr, last_addr, sizeof(struct
in_addr));
memdup() and newdup() are cool...
Post by Patrik Flykt
+struct DHCPClient;
+typedef struct DHCPClient DHCPClient;
The struct line is implied by the typedef line, you can remove it.
Post by Patrik Flykt
+
+int dhcp_client_set_request_option(DHCPClient *client, uint8_t option);
+int dhcp_client_set_request_address(DHCPClient *client,
+ struct in_addr *last_address);
+int dhcp_client_set_index(DHCPClient *client, int interface_index);
+
+DHCPClient *dhcp_client_new(void);
If this is public API these calls should have a "sd_" prefix...

Lennart
--
Lennart Poettering, Red Hat
Patrik Flykt
2013-11-15 09:21:40 UTC
Permalink
Hi,
Post by Lennart Poettering
Hmm, "struct in_addr" contains nothing but a uint32_t. It really sounds
overkill allocating such a structure individually on the
stack. Shouldn't this structure be directly inside DHCPClient? Or
actually, do we really want to use struct in_addr at all? It sounds so
useless, a "uint_32_t" sounds so much simpler, and easier to use...
Agree, uint32_t is much simpler here in the DHCPv4 case. For v6
something like a struct is needed, but that's another and later story.

Cheers,

Patrik
Lennart Poettering
2013-11-14 00:42:04 UTC
Permalink
Hi,
This patch set implements a DHCPv4 client library named libsystemd-dhcp
to be used with systemd-network.
The code implements the DHCP protocol from the INIT state up to expiry,
T1 and T2 timer setting, but does nothing in response to the IP address
reacquisition T1 and T2 timers being triggered. Expiry timer is followed
and the DHCP client stopped when it triggers. INIT-REBOOT state is also
not yet implemented; both these missing parts are to be implemented next.
The code assumes that the caller can figure out the interface to use.
The code expects to be handed an sd_event loop, a MAC address and an
interface index. The outcome is a notification of an IP address being
acquired and the callback provider is expected to set the IP address,
netmask and default gateway. Higher level DHCP options such as DNS and
NTP servers are requested from the server by default, but the response
is not yet collected anywhere. I also don't know how detailed callback
information is needed, is there for example any need to know the protocol
state transitions or messages sent/received.
I'd keep things minimal. But the addition DHCP metadata like NTP servers
we certainly should pass back to the user of the lib. However, I'd very
much prefer doing this high-level instead of low-level, i.e. provide
calls such as sd_dhcp_client_get_ntp() to get a list of NTP servers, and
so on...

I do like the code and where this is going!

Lennart
--
Lennart Poettering, Red Hat
Loading...