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

[Patrik Flykt]

	Hi,

On Tue, 2013-11-26 at 00:23 +0100, Lennart Poettering wrote:
> 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...

This one would be the minimum IPv4 packet size, 576 bytes. Should I go
with alloca() here?

> > +
> > +        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.

Ok.

> > -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...

I'll fix...

	Patrik