Send connman mailing list submissions to
[email protected]
To subscribe or unsubscribe via the World Wide Web, visit
https://lists.01.org/mailman/listinfo/connman
or, via email, send a message with subject or body 'help' to
[email protected]
You can reach the person managing the list at
[email protected]
When replying, please edit your Subject line so it is more specific
than "Re: Contents of connman digest..."
Today's Topics:
1. Re: [PATCH 03/27] shared: Add low-level ARP functions
(Daniel Wagner)
2. Re: [PATCH 01/27] shared: Add functions for random number
generation (Daniel Wagner)
3. Re: [PATCH 04/27] shared/arp: Add function
get_interface_mac_address() (Daniel Wagner)
4. Re: [PATCH 05/27] shared/arp: Add random_ip() (Daniel Wagner)
5. Re: [PATCH 06/27] inet: Add function connman_inet_is_ifup()
(Daniel Wagner)
6. Re: [PATCH 08/27] doc: Add documentation for
AddressConflictDetection (Daniel Wagner)
7. Re: [PATCH 09/27] acd: add struct acd_host (Daniel Wagner)
8. Re: [PATCH 11/27] acd: add send_probe_packet (Daniel Wagner)
9. Re: [PATCH 12/27] acd: add send_announce_packet (Daniel Wagner)
----------------------------------------------------------------------
Message: 1
Date: Mon, 16 Apr 2018 16:28:59 +0200
From: Daniel Wagner <[email protected]>
To: Christian Spielberger <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH 03/27] shared: Add low-level ARP functions
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8; format=flowed
Hi Christian,
On 04/11/2018 04:00 PM, Christian Spielberger wrote:
> We took the local functions for sending ARP packets in gdhcp and put them
> into new source files src/shared/arp.[h|c]. This will be helpful for ACD
> (Address Conflict Detection).
> +++ b/src/shared/arp.h
> @@ -0,0 +1,42 @@
> +/*
> + *
> + * Connection Manager
> + *
> + * based on IPv4 Local Link library with GLib integration,
> + * Copyright (C) 2009-2010 Aldebaran Robotics. All rights reserved.
> + *
> + * Copyright (C) 2018 Commend International. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#ifndef SHARED_ARP_H
> +#define SHARED_ARP_H
> +
> +#include <stdint.h>
> +
> +/* IPv4 Link-Local (RFC 3927), IPv4 Address Conflict Detection (RFC 5227) */
> +#define PROBE_WAIT 1
> +#define PROBE_NUM 3
> +#define PROBE_MIN 1
> +#define PROBE_MAX 2
> +#define ANNOUNCE_WAIT 2
> +#define ANNOUNCE_NUM 2
> +#define ANNOUNCE_INTERVAL 2
> +#define MAX_CONFLICTS 10
> +#define RATE_LIMIT_INTERVAL 60
> +#define DEFEND_INTERVAL 10
> +
> +int send_arp_packet(uint8_t* source_eth, uint32_t source_ip,
> + uint32_t target_ip, int ifindex);
> +int arp_socket(int ifindex);
> +
> +#endif
Let's prefix all these function with 'arp_', e.g.
arp_send_packet()
arp_socket()
Thanks,
Daniel
------------------------------
Message: 2
Date: Mon, 16 Apr 2018 16:33:33 +0200
From: Daniel Wagner <[email protected]>
To: Christian Spielberger <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH 01/27] shared: Add functions for random number
generation
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8; format=flowed
Hi Christian,
On 04/11/2018 04:00 PM, Christian Spielberger wrote:
> ACD needs random IPv4 addresses as fallback (IPv4LL address) and random delays
> between sent ARP probe and ARP announce packets. Hence, this patch moves
> ipv4ll_random_delay_ms() to a new file src/shared/random.[h|c] in order to
> be available generally.
> --- /dev/null
> +++ b/src/shared/random.h
> @@ -0,0 +1,30 @@
> +/*
> + *
> + * Random number generation library
> + *
> + * based on IPv4 Local Link library with GLib integration,
> + * Copyright (C) 2009-2010 Aldebaran Robotics. All rights reserved.
> + *
> + * Copyright (C) 2018 Commend International. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#ifndef SHARED_RANDOM_H
> +#define SHARED_RANDOM_H
> +
> +#include <stdint.h>
> +
> +int get_random(uint64_t *val);
> +void cleanup_random(void);
> +unsigned int random_delay_ms(unsigned int secs);
> +
> +#endif
Prefix these function with 'random_':
random_get()
random_cleanup()
random_delay_ms()
Maybe someone has a better idea for 'random_get()'. I'd to have all the
shared code file properly prefixed to avoid clashes.
Thanks,
Daniel
------------------------------
Message: 3
Date: Mon, 16 Apr 2018 16:38:44 +0200
From: Daniel Wagner <[email protected]>
To: Christian Spielberger <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH 04/27] shared/arp: Add function
get_interface_mac_address()
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8; format=flowed
Hi Christian,
On 04/11/2018 04:00 PM, Christian Spielberger wrote:
> Following ACD code will need this utility function. So we moved it to
> src/shared/arp.c.
> ---
> gdhcp/client.c | 32 --------------------------------
> src/shared/arp.c | 38 ++++++++++++++++++++++++++++++++++++++
> src/shared/arp.h | 2 ++
> 3 files changed, 40 insertions(+), 32 deletions(-)
>
> diff --git a/gdhcp/client.c b/gdhcp/client.c
> index 14c83b6..e36c4e0 100644
> --- a/gdhcp/client.c
> +++ b/gdhcp/client.c
> @@ -607,38 +607,6 @@ static gboolean send_announce_packet(gpointer dhcp_data)
> return TRUE;
> }
>
> -static void get_interface_mac_address(int index, uint8_t *mac_address)
> -{
> - struct ifreq ifr;
> - int sk, err;
> -
> - sk = socket(PF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0);
> - if (sk < 0) {
> - perror("Open socket error");
> - return;
> - }
> -
> - memset(&ifr, 0, sizeof(ifr));
> - ifr.ifr_ifindex = index;
> -
> - err = ioctl(sk, SIOCGIFNAME, &ifr);
> - if (err < 0) {
> - perror("Get interface name error");
> - goto done;
> - }
> -
> - err = ioctl(sk, SIOCGIFHWADDR, &ifr);
> - if (err < 0) {
> - perror("Get mac address error");
> - goto done;
> - }
> -
> - memcpy(mac_address, ifr.ifr_hwaddr.sa_data, 6);
> -
> -done:
> - close(sk);
> -}
> -
> void g_dhcpv6_client_set_retransmit(GDHCPClient *dhcp_client)
> {
> if (!dhcp_client)
> diff --git a/src/shared/arp.c b/src/shared/arp.c
> index e6c06fd..f43cbd1 100644
> --- a/src/shared/arp.c
> +++ b/src/shared/arp.c
> @@ -19,17 +19,22 @@
> */
> #include <string.h>
> #include <stdlib.h>
> +#include <stdio.h>
> #include <errno.h>
> #include <unistd.h>
>
> #include <sys/time.h>
> #include <sys/socket.h>
> #include <sys/types.h>
> +#include <sys/ioctl.h>
> +
> #include <netpacket/packet.h>
> #include <net/ethernet.h>
> #include <netinet/if_ether.h>
> #include <net/if_arp.h>
>
> +#include <linux/if.h>
> +
> #include <arpa/inet.h>
Isn't arpa/inet.h an Glibc header file? If so you should put the linux
header hafter the glibc header. See bdfb3526466f ("tethering: Reorder
header includes"):
Avoid compile errors due to unsupported header include order with
newer kernels (>=4.15). We should import the libc header files first
and then the Linux header files in user space applications.
Thanks,
Daniel
------------------------------
Message: 4
Date: Mon, 16 Apr 2018 16:40:15 +0200
From: Daniel Wagner <[email protected]>
To: Christian Spielberger <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH 05/27] shared/arp: Add random_ip()
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8; format=flowed
Hi Christian,
On 04/11/2018 04:00 PM, Christian Spielberger wrote:
> Following feature ACD also needs a random IPv4 address as fallback. We move
> the function ipv4ll_random_ip() from gdhcp/ipv4ll.h to src/shared/random.h.
> The gdhcp/ipv4ll.h is now empty and can be removed.
> --- a/src/shared/arp.h
> +++ b/src/shared/arp.h
> @@ -35,10 +35,15 @@
> #define RATE_LIMIT_INTERVAL 60
> #define DEFEND_INTERVAL 10
>
> +/* 169.254.0.0 */
> +#define LINKLOCAL_ADDR 0xa9fe0000
> +
> int send_arp_packet(uint8_t* source_eth, uint32_t source_ip,
> uint32_t target_ip, int ifindex);
> int arp_socket(int ifindex);
>
> +uint32_t random_ip(void);
Rename this to arp_random_ip()
Thanks,
Daniel
------------------------------
Message: 5
Date: Mon, 16 Apr 2018 16:46:02 +0200
From: Daniel Wagner <[email protected]>
To: Christian Spielberger <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH 06/27] inet: Add function connman_inet_is_ifup()
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8; format=flowed
Hi Christian,
On 04/11/2018 04:00 PM, Christian Spielberger wrote:
> Add function connman_inet_is_ifup() which checks if an interface is up. It
> will
> be used when initializing ACD.
> ---
> include/inet.h | 1 +
> src/inet.c | 34 ++++++++++++++++++++++++++++++++++
> 2 files changed, 35 insertions(+)
>
> diff --git a/include/inet.h b/include/inet.h
> index 6482934..9c1918f 100644
> --- a/include/inet.h
> +++ b/include/inet.h
> @@ -38,6 +38,7 @@ char *connman_inet_ifname(int index);
>
> int connman_inet_ifup(int index);
> int connman_inet_ifdown(int index);
> +bool connman_inet_is_ifup(int index);
Since the all the other connman_inet do return an integer as return
value I suggest you stay consistent and use also an int as return code.
The connman_inet_*() function with a bool as return are expected to be
used in a condition. So this is about consistency/style. Nothing really
wrong with your version.
Thanks,
Daniel
------------------------------
Message: 6
Date: Mon, 16 Apr 2018 16:47:56 +0200
From: Daniel Wagner <[email protected]>
To: Christian Spielberger <[email protected]>
Cc: [email protected], Peter Meerwald-Stadler <[email protected]>
Subject: Re: [PATCH 08/27] doc: Add documentation for
AddressConflictDetection
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8; format=flowed
Hi,
On 04/11/2018 04:00 PM, Christian Spielberger wrote:
> From: Peter Meerwald-Stadler <[email protected]>
>
> Signed-off-by: Peter Meerwald-Stadler <[email protected]>
> ---
> doc/connman.conf.5.in | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/doc/connman.conf.5.in b/doc/connman.conf.5.in
> index 809a0a4..be31902 100644
> --- a/doc/connman.conf.5.in
> +++ b/doc/connman.conf.5.in
> @@ -161,6 +161,16 @@ Default value is true.
> Automatically connect roaming services. This is not recommended unless you
> know
> you won't have any billing problem.
> Default value is false.
> +.TP
> +.BI AddressConflictDetection=true\ \fR|\fB\ false
> +Enable or disable the implementation of IPv4 address conflict detection
> +according to RFC5227. ConnMan will send probe ARP packets to see if an
> +IPv4 address is already in use before assigning the address to an interface.
> +If an address conflict occurs for a statically configured address, an IPv4LL
> +address will be chosen instead (according to RFC3927). If an address conflict
> +occurs for an address offered via DHCP, ConnMan send a DHCP DECLINE once and
> +for the second conflict resort to finding an IPv4LL address.
> +Default value is false.
Shouldn't we enable it on default? This feature sounds useful for everybody.
Thanks,
Daniel
------------------------------
Message: 7
Date: Mon, 16 Apr 2018 16:51:43 +0200
From: Daniel Wagner <[email protected]>
To: Christian Spielberger <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH 09/27] acd: add struct acd_host
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8; format=flowed
Hi Christian,
On 04/11/2018 04:00 PM, Christian Spielberger wrote:
> Adds an enum for the ACD state machine, the ACD struct, a function for
> allocation and initialization of the struct.
> ---
> include/acd.h | 4 ++++
> src/acd.c | 70
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 74 insertions(+)
>
> diff --git a/include/acd.h b/include/acd.h
> index c347d77..08f9ec9 100644
> --- a/include/acd.h
> +++ b/include/acd.h
> @@ -23,6 +23,8 @@
> #ifndef __CONNMAN_ACD_H
> #define __CONNMAN_ACD_H
>
> +#include <stdint.h>
> +
> #ifdef __cplusplus
> extern "C" {
> #endif
> @@ -31,6 +33,8 @@ struct _acd_host;
>
> typedef struct _acd_host acd_host;
>
> +acd_host *acdhost_new(int ifindex);
Rename it to acd_host_new()
> +
> #ifdef __cplusplus
> }
> #endif
> diff --git a/src/acd.c b/src/acd.c
> index 1653d2f..bbc1558 100644
> --- a/src/acd.c
> +++ b/src/acd.c
> @@ -17,3 +17,73 @@
> *
> */
>
> +#include <connman/acd.h>
> +#include <connman/log.h>
> +#include <connman/inet.h>
> +#include <glib.h>
> +#include "src/shared/arp.h"
> +
> +typedef enum _acd_state {
> + ACD_PROBE,
> + ACD_ANNOUNCE,
> + ACD_MONITOR,
> + ACD_DEFEND,
> +} ACDState;
Check coding-style.txt on 'M11: Naming of enums' So this would be
enum acd_state {
ACD_STATE_PROBE,
ACD_STATE_ANNOUNCE,
...
};
Note no typedef.
> +
> +struct _acd_host {
struct acd_host
Thanks,
Danie
------------------------------
Message: 8
Date: Mon, 16 Apr 2018 16:53:11 +0200
From: Daniel Wagner <[email protected]>
To: Christian Spielberger <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH 11/27] acd: add send_probe_packet
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8; format=flowed
Hi Christian,
On 04/11/2018 04:00 PM, Christian Spielberger wrote:
> Adds a function for sending ARP probe packets and a timeout callback function
> for invoking send_probe_packet repeatedly.
> ---
> src/acd.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 52 insertions(+)
>
> diff --git a/src/acd.c b/src/acd.c
> index 7dcad2e..173ce47 100644
> --- a/src/acd.c
> +++ b/src/acd.c
> @@ -22,9 +22,11 @@
> #include <connman/inet.h>
> #include <glib.h>
> #include "src/shared/arp.h"
> +#include "src/shared/random.h"
Move this after the standard headers.
Thanks,
Daniel
------------------------------
Message: 9
Date: Mon, 16 Apr 2018 16:56:33 +0200
From: Daniel Wagner <[email protected]>
To: Christian Spielberger <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH 12/27] acd: add send_announce_packet
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8
Hi Christian,
On 04/11/2018 04:00 PM, Christian Spielberger wrote:
> Adding function for sending announce packets and only function headers of
> the timeout callbacks acd_announce_timeout for repeated announce packets and
> acd_defend_timeout for switching from DEFEND state back to MONITOR state.
> +static gboolean send_announce_packet(gpointer acd_data)
> +{
> + acd_host *acd = acd_data;
> +
> + debug(acd, "sending ACD announce request");
> +
> + send_arp_packet(acd->mac_address,
> + acd->requested_ip,
> + acd->requested_ip,
> + acd->ifindex);
> +
> + remove_timeout(acd);
> +
> + if (acd->state == ACD_DEFEND)
> + acd->timeout = g_timeout_add_seconds_full(G_PRIORITY_HIGH,
> + DEFEND_INTERVAL,
> + acd_defend_timeout,
> + acd,
> + NULL);
> + else
> + acd->timeout = g_timeout_add_seconds_full(G_PRIORITY_HIGH,
> + ANNOUNCE_INTERVAL,
> + acd_announce_timeout,
> + acd,
> + NULL);
This looks a bit ugly. What about something like
interval = acd->state == ACD_DEFEND? DEFEND_INTERVAL : ANNOUCE_INTERVAL;
acd->timeout = g_timeout_add_seconds_full(G_PRIORITY_HIGH,
interval,
acd_announce_timeout,
acd,
NULL);
Thanks,
Daniel
------------------------------
Subject: Digest Footer
_______________________________________________
connman mailing list
[email protected]
https://lists.01.org/mailman/listinfo/connman
------------------------------
End of connman Digest, Vol 30, Issue 15
***************************************
