On Wed, Nov 13, 2013 at 11:22:52PM +0200, Patrik Flykt wrote: > 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; 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 ... > + 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 ... > + } > + > + if (type != DHCP_ACK) > + goto error; Is err set correctly here? > + > + 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 ... > + } > + > + 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 ... > + > + return err; ... and remove the part below. > + > +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; -1 not 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; > Zbyszek _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel