[systemd-devel] [PATCH v2 20/26] dhcp: Handle received DHCP Offer message

[Lennart Poettering]

On Mon, 25.11.13 09:13, Patrik Flykt (patrik.flykt at linux.intel.com) wrote:

> +static int client_receive_raw_message(sd_event_source *s, int fd,
> +                                      uint32_t revents, void *userdata)
> +{
> +        DHCPClient *client = userdata;
> +        int len, buflen;
> +        uint8_t *buf;
> +        uint8_t tmp;
> +        DHCPPacket *message;
> +
> +        buflen = sizeof(DHCPPacket) + DHCP_CLIENT_MIN_OPTIONS_SIZE;
> +        buf = malloc0(buflen);
> +        if (!buf) {
> +                read(fd, &tmp, 1);
> +                return 0;
> +        }

Hmm, how long is this message? Given that you don't keep the memory
around, why not just allocate this on the stack? Either with alloca, or
even directly as an uint8_t array? Given that this seems to be
relatively short, this should not be an issue and is one error source
less...

> +
> +        len = read(fd, buf, buflen);
> +        if (len < 0)
> +                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 = -1;
> +                        client->receive_message =
> +
> sd_event_source_unref(client->receive_message);


You should unref the source before closing the fd, as this internally
still references the fd.

> -int dhcp_network_send_raw_packet(int index, void *packet, int len)
> +int dhcp_network_send_raw_socket(int s, const union sockaddr_union *link,
> +                                 void *packet, int len)

Hmm, what's the story regarding blocking here? What happens if the
socket is full? This call will block then, which sucks. Also, if you are
not using SOCK_NONBLOCK on these sockets, epoll behaviour can be weird,
as the socket layer takes the freedom to wake you up sometimes without
any packet to read. (We had the issue in Avahi).

It sounds to me as if you should set SOCK_NONBLOCK and then set a socket
send buffer (SO_SNDBUF) as large as useful, and simply consider it a
real error if you the ever get EAGAIN on sending...

Lennart

-- 
Lennart Poettering, Red Hat