Hi, On Fri, 2014-09-26 at 15:15 -0500, Dan Williams wrote: > /* RFC2132 section 4.1.1: > The client MUST include its hardware address in the ’chaddr’ > field, if > - necessary for delivery of DHCP reply messages. > + necessary for delivery of DHCP reply messages. Non-Ethernet > + interfaces will leave 'chaddr' empty and use the client identifier > + instead (eg, RFC 4390 section 2.1). > */ > - memcpy(&packet->dhcp.chaddr, &client->client_id.mac_addr, ETH_ALEN); > + if (client->mac_addr_len == ETH_ALEN) > + memcpy(&packet->dhcp.chaddr, &client->mac_addr, ETH_ALEN);
Sorry about the late review, but shouldn't this be more generic if written: if (client->mac_addr_len) memcpy(&packet->dhcp.chaddr, &client->mac_addr, client->mac_addr_len); With that, all cases are covered. Which, AFAIK, are none in addition to ethernet. An assert() should check the given MAC address length at some point in the code so that it cannot be > 16. ... > diff --git a/src/libsystemd-network/sd-dhcp-server.c > b/src/libsystemd-network/sd-dhcp-server.c > index a6d6178..24fedd2 100644 > --- a/src/libsystemd-network/sd-dhcp-server.c > +++ b/src/libsystemd-network/sd-dhcp-server.c > @@ -388,16 +388,16 @@ static int server_message_init(sd_dhcp_server *server, > DHCPPacket **ret, > assert(IN_SET(type, DHCP_OFFER, DHCP_ACK, DHCP_NAK)); > > packet = malloc0(sizeof(DHCPPacket) + req->max_optlen); > if (!packet) > return -ENOMEM; > > r = dhcp_message_init(&packet->dhcp, BOOTREPLY, > - be32toh(req->message->xid), type, > req->max_optlen, > - &optoffset); > + be32toh(req->message->xid), type, ARPHRD_ETHER, > + req->max_optlen, &optoffset); > if (r < 0) > return r; > > packet->dhcp.flags = req->message->flags; > packet->dhcp.giaddr = req->message->giaddr; > memcpy(&packet->dhcp.chaddr, &req->message->chaddr, ETH_ALEN); Unrelated to your patch, I think a bug is lurking on the last line. I believe the full 16 bytes of dhcp.chaddr should be copied from the client's message according to the table in Section 4.3.1 in RFC 2131. Including any possible garbage the client left in its message. I wonder how many client implementation break due to such a change, though... Cheers, Patrik _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel