Re: [systemd-devel] [PATCH v2 20/26] dhcp: Handle received DHCP Offer message
Hi,
On Tue, 2013-11-26 at 00:23 +0100, Lennart Poettering wrote:
On Mon, 25.11.13 09:13, Patrik Flykt (patrik.fl...@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
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH v2 20/26] dhcp: Handle received DHCP Offer message
On Wed, 27.11.13 17:13, Patrik Flykt (patrik.fl...@linux.intel.com) wrote:
+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?
As a rule of thumb I'd alway prefer stack allocation for things 2K or
so... In userspace that should be totally OK, and if you never need this
buffer outside of this call, then stack allocation should be fine.
Note that allocating this as explicit array on the stack is usually the
better option than alloca(), since alloca() can get very confusing when
used inside of loops.
Hence:
uint8_t buf[sizeof(DHCPPacket) + DHCP_CLIENT_MIN_OPTIONS_SIZE];
sounds better, if you can avoid:
void *buf;
buf = alloca0(sizeof(DHCPPacket) + DHCP_CLIENT_MIN_OPTIONS_SIZE);
which is still better than:
_cleanup_free_ void *buf;
buf = malloc0(sizeof(DHCPPacket) + DHCP_CLIENT_MIN_OPTIONS_SIZE);
if (!buf)
...
All of course only if the the size is not huge, and if you never need
the thing outside of the local scope...
Lennart
--
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH v2 20/26] dhcp: Handle received DHCP Offer message
On Mon, 25.11.13 09:13, Patrik Flykt (patrik.fl...@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
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
