Send connman mailing list submissions to
        connman@lists.01.org

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
        connman-requ...@lists.01.org

You can reach the person managing the list at
        connman-ow...@lists.01.org

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 <w...@monom.org>
To: Christian Spielberger <christian.spielber...@gmail.com>
Cc: connman@lists.01.org
Subject: Re: [PATCH 03/27] shared: Add low-level ARP functions
Message-ID: <58c1b049-6fbc-6f00-f5eb-875f78da8...@monom.org>
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 <w...@monom.org>
To: Christian Spielberger <christian.spielber...@gmail.com>
Cc: connman@lists.01.org
Subject: Re: [PATCH 01/27] shared: Add functions for random number
        generation
Message-ID: <569783bb-2b3b-bded-de81-085dc49e7...@monom.org>
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 <w...@monom.org>
To: Christian Spielberger <christian.spielber...@gmail.com>
Cc: connman@lists.01.org
Subject: Re: [PATCH 04/27] shared/arp: Add function
        get_interface_mac_address()
Message-ID: <f7bf1b8d-6044-e24d-7980-90b156523...@monom.org>
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 <w...@monom.org>
To: Christian Spielberger <christian.spielber...@gmail.com>
Cc: connman@lists.01.org
Subject: Re: [PATCH 05/27] shared/arp: Add random_ip()
Message-ID: <4275ea3f-2d72-fe52-6ab5-685495e3d...@monom.org>
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 <w...@monom.org>
To: Christian Spielberger <christian.spielber...@gmail.com>
Cc: connman@lists.01.org
Subject: Re: [PATCH 06/27] inet: Add function connman_inet_is_ifup()
Message-ID: <7f8526d6-1865-7c08-2b4d-9b917f790...@monom.org>
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 <w...@monom.org>
To: Christian Spielberger <christian.spielber...@gmail.com>
Cc: connman@lists.01.org, Peter Meerwald-Stadler <pme...@pmeerw.net>
Subject: Re: [PATCH 08/27] doc: Add documentation for
        AddressConflictDetection
Message-ID: <ca54dd61-d0df-60c8-cdd5-9f8c9a1ed...@monom.org>
Content-Type: text/plain; charset=utf-8; format=flowed

Hi,

On 04/11/2018 04:00 PM, Christian Spielberger wrote:
> From: Peter Meerwald-Stadler <pme...@pmeerw.net>
> 
> Signed-off-by: Peter Meerwald-Stadler <pme...@pmeerw.net>
> ---
>   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 <w...@monom.org>
To: Christian Spielberger <christian.spielber...@gmail.com>
Cc: connman@lists.01.org
Subject: Re: [PATCH 09/27] acd: add struct acd_host
Message-ID: <adc41b9c-bb40-4757-24f3-b0b05e0a9...@monom.org>
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 <w...@monom.org>
To: Christian Spielberger <christian.spielber...@gmail.com>
Cc: connman@lists.01.org
Subject: Re: [PATCH 11/27] acd: add send_probe_packet
Message-ID: <1e31eab7-c571-df1f-219d-99f08174a...@monom.org>
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 <w...@monom.org>
To: Christian Spielberger <christian.spielber...@gmail.com>
Cc: connman@lists.01.org
Subject: Re: [PATCH 12/27] acd: add send_announce_packet
Message-ID: <196ba7a4-1da5-13a0-cd7e-5d3e21966...@monom.org>
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
connman@lists.01.org
https://lists.01.org/mailman/listinfo/connman


------------------------------

End of connman Digest, Vol 30, Issue 15
***************************************

Reply via email to