Hi Jukka,
> In order to avoid clang error, just use memcpy() when reading
> or setting variable according to memory buffer read from wire.
> ---
> gdhcp/client.c | 14 +++++++-------
> gdhcp/common.c | 2 +-
> gdhcp/server.c | 9 ++++-----
> 3 files changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/gdhcp/client.c b/gdhcp/client.c
> index cf04ced..d5f1daa 100644
> --- a/gdhcp/client.c
> +++ b/gdhcp/client.c
> @@ -1340,7 +1340,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);
> + memcpy(&lease_seconds, option_u8, sizeof(lease_seconds));
I am still confused on how a option_u8 can become a uint32 this way.
This looks fishy and we might just need better naming.
However if this a uint32 then I bet we need endian conversion here as
well.
> lease_seconds = ntohl(lease_seconds);
> /* paranoia: must not be prone to overflows */
> lease_seconds &= 0x0fffffff;
> @@ -1518,14 +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);
> + uint16_t val_u16;
> + memcpy(&val_u16, option, sizeof(val_u16));
This is the same thing, I am missing endian conversion.
> dest += sprintf(dest, "%u", ntohs(val_u16));
I see it here, but why don't we convert right away into a correct value.
> break;
> }
> case OPTION_U32: {
> - uint32_t val_u32 = dhcp_get_unaligned(
> - (uint32_t *) option);
> + uint32_t val_u32;
> + memcpy(&val_u32, option, sizeof(val_u32));
> dest += sprintf(dest, type == OPTION_U32 ? "%lu" :
> "%ld", (unsigned long) ntohl(val_u32));
Same here and this one worries me even more. What is unsigned long vs
uint32 mixture.
> break;
> @@ -1924,8 +1924,8 @@ 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);
> + memcpy(&dhcp_client->server_ip, option_u8,
> + sizeof(dhcp_client->server_ip));
I do not get the option_u8 naming. That is horrible bad naming if it is
just a pointer. Maybe it should just point to const void *.
> dhcp_client->requested_ip = packet.yiaddr;
>
> dhcp_client->state = REQUESTING;
> diff --git a/gdhcp/common.c b/gdhcp/common.c
> index 8d5c284..ae062f3 100644
> --- a/gdhcp/common.c
> +++ b/gdhcp/common.c
> @@ -284,7 +284,7 @@ void dhcp_add_simple_option(struct dhcp_packet *packet,
> uint8_t code,
> data <<= 8 * (4 - len);
> #endif
>
> - dhcp_put_unaligned(data, (uint32_t *)(option + OPT_DATA));
> + memcpy(option + OPT_DATA, &data, len);
Same here. The magic endian above is bad. We should just have a put_u32
helper.
> dhcp_add_binary_option(packet, option);
>
> return;
> diff --git a/gdhcp/server.c b/gdhcp/server.c
> index a342985..6dec651 100644
> --- a/gdhcp/server.c
> +++ b/gdhcp/server.c
> @@ -664,17 +664,16 @@ 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;
> + memcpy(&server_nid, server_id_option, sizeof(server_nid));
> if (server_nid != dhcp_server->server_nip)
> return TRUE;
> }
>
> request_ip_option = dhcp_get_option(&packet, DHCP_REQUESTED_IP);
> if (request_ip_option)
> - requested_nip = dhcp_get_unaligned(
> - (uint32_t *) request_ip_option);
> + memcpy(&requested_nip, request_ip_option,
> + sizeof(requested_nip));
Why don't we have a working get_le32 that handles this all for us?
> lease = find_lease_by_mac(dhcp_server, packet.chaddr);
>
Regards
Marcel
_______________________________________________
connman mailing list
[email protected]
http://lists.connman.net/listinfo/connman
