Re: [systemd-devel] [PATCH v2 23/26] dhcp: Process DHCP Ack/Nak message

2013-11-27 Thread Patrik Flykt
On Tue, 2013-11-26 at 00:26 +0100, Lennart Poettering wrote:
> On Mon, 25.11.13 09:13, Patrik Flykt (patrik.fl...@linux.intel.com) wrote:
> 
> > -static int client_receive_offer(DHCPClient *client,
> > -DHCPPacket *offer, int len)
> > +static int client_verify_headers(DHCPClient *client,
> > + DHCPPacket *message, int len)
> 
> Thhis all looks like "size_t len" is the right param here, not "int".

Ok.

> > +lease = new0(DHCPLease, 1);
> > +if (!lease)
> > +return -ENOBUFS;
> 
> OOM should result in ENOMEM, nothing else please. ENOBUFS is something
> else: it indicates some local limit to be hit, not a global memory
> shortage.

Forgotten from the previous fixing, sorry!

Cheers,

Patrik


___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel


Re: [systemd-devel] [PATCH v2 23/26] dhcp: Process DHCP Ack/Nak message

2013-11-25 Thread Lennart Poettering
On Mon, 25.11.13 09:13, Patrik Flykt (patrik.fl...@linux.intel.com) wrote:

> -static int client_receive_offer(DHCPClient *client,
> -DHCPPacket *offer, int len)
> +static int client_verify_headers(DHCPClient *client,
> + DHCPPacket *message, int len)

Thhis all looks like "size_t len" is the right param here, not "int".

> +lease = new0(DHCPLease, 1);
> +if (!lease)
> +return -ENOBUFS;

OOM should result in ENOMEM, nothing else please. ENOBUFS is something
else: it indicates some local limit to be hit, not a global memory
shortage.

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel


[systemd-devel] [PATCH v2 23/26] dhcp: Process DHCP Ack/Nak message

2013-11-24 Thread 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.
---
v2: - previous 24/28
- replace 'err' with 'r' and fix fd as suggested
- same thing with _cleanup_free_ for a lease as in the previous one,
  usage of the allocation depends on success/failure
- rename DHCP_EVENT_NAK to DHCP_EVENT_NO_LEASE so it's easier to
  understand

 src/dhcp/client.c|  136 ++
 src/systemd/sd-dhcp-client.h |6 ++
 2 files changed, 117 insertions(+), 25 deletions(-)

diff --git a/src/dhcp/client.c b/src/dhcp/client.c
index 1c6602c..bee1a3a 100644
--- a/src/dhcp/client.c
+++ b/src/dhcp/client.c
@@ -136,6 +136,11 @@ int sd_dhcp_client_set_mac(DHCPClient *client, const 
struct ether_addr *addr)
 return 0;
 }
 
+static int client_notify(DHCPClient *client, int event)
+{
+return 0;
+}
+
 static int client_stop(DHCPClient *client, int error)
 {
 assert_return(client, -EINVAL);
@@ -157,6 +162,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;
@@ -164,7 +170,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:
@@ -477,41 +482,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 + be16toh(offer->udp.len) > len ||
-client_checksum(&offer->ip.ttl, be16toh(offer->udp.len) + 12))
+if (hdrlen + be16toh(message->udp.len) > len ||
+client_checksum(&message->ip.ttl, be16toh(message->udp.len) + 12))
 return -EINVAL;
 
-if (be16toh(offer->udp.source) != DHCP_PORT_SERVER ||
-be16toh(offer->udp.dest) != DHCP_PORT_CLIENT)
+if (be16toh(message->udp.source) != DHCP_PORT_SERVER ||
+be16toh(message->udp.dest) != DHCP_PORT_CLIENT)
 return -EINVAL;
 
-if (offer->dhcp.op != BOOTREPLY)
+if (message->dhcp.op != BOOTREPLY)
 return -EINVAL;
 
-if (be32toh(offer->dhcp.xid) != client->xid)
+if (be32toh(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 -ENOMEM;
@@ -539,11 +555,60 @@ error:
 return -ENOMSG;
 }
 
+static int client_receive_ack(DHCPClient *client, DHCPPacket *offer, int len)
+{
+int r;
+DHCPLease *lease;
+
+r = client_verify_headers(client, offer, len);
+if (r < 0)
+return r;
+
+lease = new0(DHCPLease, 1);
+if (!lease)
+return -ENOBUFS;
+
+len = len - DHCP_IP_UDP_SIZE;
+r = dhcp_option_parse(&offer->dhcp, len, client_parse_offer, lease);
+
+if (r != DHCP_ACK)
+goto error;
+
+lease->address = offer->dhcp.yiaddr;
+
+if (lease->address == INADDR_ANY ||
+lease->server_address == INADDR_ANY ||
+lease->subnet_mask == INADDR_ANY || lease->lifetime == 0) {
+r = -ENOMSG;
+goto error;
+}
+
+r = 0;
+if (client->