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 @@
>
> #include <glib.h>
>
> +#include <connman/unaligned.h>
> #include "gdhcp.h"
> #include "common.h"
> #include "ipv4ll.h"
> @@ -1340,8 +1341,7 @@ static uint32_t get_lease(struct dhcp_packet *packet)
> if (option_u8 == NULL)
> return 3600;
>
> - lease_seconds = dhcp_get_unaligned((uint32_t *) option_u8);
> - lease_seconds = ntohl(lease_seconds);
> + lease_seconds = get_unaligned_be32(option_u8);
this looks way nicer now.
> /* paranoia: must not be prone to overflows */
> lease_seconds &= 0x0fffffff;
> if (lease_seconds < 10)
> @@ -1518,16 +1518,14 @@ static char *malloc_option_value_string(uint8_t
> *option, GDHCPOptionType type)
> dest += sprint_nip(dest, "", option);
> break;
> case OPTION_U16: {
> - uint16_t val_u16 = dhcp_get_unaligned(
> - (uint16_t *) option);
> - dest += sprintf(dest, "%u", ntohs(val_u16));
> + uint16_t val_u16 = get_unaligned_be16(option);
> + dest += sprintf(dest, "%u", val_u16);
> break;
> }
> 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?
> 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?
> 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?
>
> - 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.
>
> 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?
>
> lease = find_lease_by_mac(dhcp_server, packet.chaddr);
>
Regards
Marcel
_______________________________________________
connman mailing list
connman@connman.net
http://lists.connman.net/listinfo/connman
