Hi Marcel, On 09.10.2012 17:09, Marcel Holtmann wrote:
Hi Jukka,gdhcp/client.c | 17 +++++++---------- gdhcp/common.c | 19 +++++++++++++++---- gdhcp/common.h | 16 ---------------- gdhcp/server.c | 8 ++++---- 4 files changed, 26 insertions(+), 34 deletions(-) diff --git a/gdhcp/client.c b/gdhcp/client.c index cf04ced..801d652 100644 --- a/gdhcp/client.c +++ b/gdhcp/client.c @@ -41,6 +41,7 @@ case OPTION_U32: { - uint32_t val_u32 = dhcp_get_unaligned( - (uint32_t *) option); + uint32_t val_u32 = get_unaligned_be32(option); dest += sprintf(dest, type == OPTION_U32 ? "%lu" : - "%ld", (unsigned long) ntohl(val_u32)); + "%ld", (unsigned long)val_u32);I do not get this unsigned long magic here? Why are we doing that instead of just using %u?
Not my code so don't know why it is done like that. I will fix it.
break; } case OPTION_STRING: @@ -1924,8 +1922,7 @@ static gboolean listener_event(GIOChannel *channel, GIOCondition condition, dhcp_client->retry_times = 0; option_u8 = dhcp_get_option(&packet, DHCP_SERVER_ID); - dhcp_client->server_ip = - dhcp_get_unaligned((uint32_t *) option_u8); + dhcp_client->server_ip = get_unaligned((uint32_t *)option_u8);Why is this used in native endian?
No need to convert to native endian as server_ip is sent back to wire anyway.
dhcp_client->requested_ip = packet.yiaddr; dhcp_client->state = REQUESTING; diff --git a/gdhcp/common.c b/gdhcp/common.c index 8d5c284..7cca3a3 100644 --- a/gdhcp/common.c +++ b/gdhcp/common.c @@ -36,6 +36,7 @@ #include <net/ethernet.h> #include <arpa/inet.h> +#include <connman/unaligned.h>Can we keep this gdhcp/unaligned.h for now.#include "gdhcp.h" #include "common.h" @@ -280,11 +281,21 @@ void dhcp_add_simple_option(struct dhcp_packet *packet, uint8_t code, len = dhcp_option_lengths[type & OPTION_TYPE_MASK]; option[OPT_LEN] = len; -#if __BYTE_ORDER == __BIG_ENDIAN - data <<= 8 * (4 - len); -#endif + switch (len) { + case 1: + option[OPT_DATA] = data; + break; + case 2: + put_unaligned(data, (uint16_t *)(option + OPT_DATA)); + break; + case 4: + put_unaligned(data, (uint32_t *)(option + OPT_DATA)); + break; + default: + printf("Invalid option len %d for code 0x%x\n", len, code); + return; + }Are we not missing endian conversion here. This is going back onto the wire, right?
The data parameter is suppose to be in wire format (big endian) already. I did not want to change this in order to avoid conversion errors.
- dhcp_put_unaligned(data, (uint32_t *)(option + OPT_DATA)); dhcp_add_binary_option(packet, option); return; diff --git a/gdhcp/common.h b/gdhcp/common.h index 13f49d8..c1c5e4f 100644 --- a/gdhcp/common.h +++ b/gdhcp/common.h @@ -26,22 +26,6 @@ #include "gdhcp.h" -#define dhcp_get_unaligned(ptr) \ -({ \ - struct __attribute__((packed)) { \ - typeof(*(ptr)) __v; \ - } *__p = (void *) (ptr); \ - __p->__v; \ -}) - -#define dhcp_put_unaligned(val, ptr) \ -do { \ - struct __attribute__((packed)) { \ - typeof(*(ptr)) __v; \ - } *__p = (void *) (ptr); \ - __p->__v = (val); \ -} while (0) - #define CLIENT_PORT 68 #define SERVER_PORT 67 diff --git a/gdhcp/server.c b/gdhcp/server.c index a342985..3c2d057 100644 --- a/gdhcp/server.c +++ b/gdhcp/server.c @@ -40,6 +40,7 @@ #include <glib.h> +#include <connman/unaligned.h> #include "common.h" /* 8 hours */ @@ -664,8 +665,8 @@ static gboolean listener_event(GIOChannel *channel, GIOCondition condition, server_id_option = dhcp_get_option(&packet, DHCP_SERVER_ID); if (server_id_option) { - uint32_t server_nid = dhcp_get_unaligned( - (uint32_t *) server_id_option); + uint32_t server_nid = + get_unaligned((uint32_t *)server_id_option);Same here. I do not get it. Why can we not convert this into native endian.
No need. The param is only used when comparing to server_nip which is in wire format.
if (server_nid != dhcp_server->server_nip) return TRUE; @@ -673,8 +674,7 @@ static gboolean listener_event(GIOChannel *channel, GIOCondition condition, request_ip_option = dhcp_get_option(&packet, DHCP_REQUESTED_IP); if (request_ip_option) - requested_nip = dhcp_get_unaligned( - (uint32_t *) request_ip_option); + requested_nip = get_unaligned((uint32_t *)request_ip_option);And here again. Native endian?
No need, wire format used on purpose.
lease = find_lease_by_mac(dhcp_server, packet.chaddr);Regards Marcel
Cheers, Jukka _______________________________________________ connman mailing list [email protected] http://lists.connman.net/listinfo/connman
