10:03 a.m.
Hello,
We are planning to add Address Conflict Detection RFC-5227 into connman. It will
be my task to implement the main aspects of this RFC in the next 2-3 weeks.
Now I will start by cloning the project to github. Then looking where and how
to place the ACD code. If you have any hints, please let me know!
The goal would be definitely to bring it upstream.
regards,
Christian Spielberger
https://github.com/cspiel1
www.commend.com
3:24 p.m.
Hi Christian,
sorry for the late response due $reason...
On 07.02.2018 11:03, Christian Spielberger wrote:
Hello,
We are planning to add Address Conflict Detection RFC-5227 into connman.
Nice :)
It will
be my task to implement the main aspects of this RFC in the next 2-3 weeks.
Now I will start by cloning the project to github. Then looking where and how
to place the ACD code. If you have any hints, please let me know!
I would start by looking what we have in gdhcp/client.c. There is some
code in there for L2/L3 handling. From a quick glance in the RFC it
looks like the right place to start.
The goal would be definitely to bring it upstream.
That is highly appreciated!
Thanks,
Daniel
9 a.m.
On Fri 09.02.18 16:24, Daniel Wagner wrote:
Hi Christian,
sorry for the late response due $reason...
On 07.02.2018 11:03, Christian Spielberger wrote:
> Hello,
>
> We are planning to add Address Conflict Detection RFC-5227 into connman.
Nice :)
> It will
> be my task to implement the main aspects of this RFC in the next 2-3 weeks.
> Now I will start by cloning the project to github. Then looking where and how
> to place the ACD code. If you have any hints, please let me know!
I would start by looking what we have in gdhcp/client.c. There is some
code in there for L2/L3 handling. From a quick glance in the RFC it
looks like the right place to start.
> The goal would be definitely to bring it upstream.
That is highly appreciated!
Thanks,
Daniel
Hi Daniel,
thanks for this hint! Indeed the ACD (RFC-5227) and IPv4ll (RFC-3927) seem
to have a copy paste relation. And the code for IPv4ll is useful for my
intention.
So, here more concrete questions:
- We could use the utility functions in gdhcp/ipv4ll.c for sending arp packets.
The functions ipv4ll_send_arp_packet and ipv4ll_arp_socket are not specific to
dhcp. Should I move them to another folder (src, or new arp) and rename them?
- As you stated, very useful is also gdhcp/client.c. The functions
send_probe_packet, send_announce_packet, and so on do exactly what we need for
ACD. But they are all related to dhcp because they use the structure GDHCPClient.
ACD should work for dhcp and static ipv4 configurations.
Do you want some refactoring? Clearly, the simplest way is to copy and rewrite
these functions and don't touch client.c. What do you think?
- Last question is where to trigger the ACD. I made a call-graph that shows
which functions lead to inet.c function
__connman_inet_modify_address
where the ip address is assigned to the interface. (Dot code below!) Maybe ACD
should be triggered in inet.c function
connman_inet_set_address ?
This also influences vpn-ipconfig.c and vpn-provider.c where
connman_inet_set_address is called. Or should I place it one step upward in
ipconfig.c function
__connman_ipconfig_address_add ?
regards,
Christian.
#####Call graph (use e.g. xdot to view) #############
digraph change_ipv4 {
rankdir=LR
node [shape = box]
{
modAd [label="inet.c\n__connman_inet_modify_address"]
setAd [label="inet.c\nconnman_inet_set_address"]
C [label="bridge.c\n__connman_bridge_enable"]
D [label="peer.c\nstart_dhcp_server"]
E [label="tethering.c\nsetup_tun_interface"]
F [label="tethering.c\n__connman_tethering_set_enabled"]
G [label="provider.c\nset_connected"]
H [label="network.c\ndhcp_success"]
I [label="network.c\ndhcpv6_set_addresses"]
J [label="network.c\nset_connected_manual"]
K [label="network.c\nmanual_ipv6_set"]
L [label="peer.c\ndhcp_callback"]
addAd [label="ipconfig.c\n__connman_ipconfig_address_add"]
vpnAd [label="vpn-ipconfig.c\n__vpn_ipconfig_address_add"]
vpncAd [label="vpn-provider.c\nvpn_provider_change_address"]
}
setAd -> modAd
C -> modAd
D -> modAd
E -> modAd
F -> C
addAd -> setAd
vpnAd -> setAd
vpncAd -> setAd
G -> addAd
H -> addAd
I -> addAd
J -> addAd
K -> addAd
L -> addAd
}
#####################################################
9:58 a.m.
Good Morning Christian,
So, here more concrete questions:
- We could use the utility functions in gdhcp/ipv4ll.c for sending arp packets.
The functions ipv4ll_send_arp_packet and ipv4ll_arp_socket are not specific to
dhcp. Should I move them to another folder (src, or new arp) and rename them?
Indeed the ipv4ll_ are not specific to dhcp but currently we just have
one user. If you need it in src, then yes please move it there.
- As you stated, very useful is also gdhcp/client.c. The functions
send_probe_packet, send_announce_packet, and so on do exactly what we need for
ACD. But they are all related to dhcp because they use the structure GDHCPClient.
ACD should work for dhcp and static ipv4 configurations.
Do you want some refactoring? Clearly, the simplest way is to copy and rewrite
these functions and don't touch client.c. What do you think?
Please don't do code duplication because it is a maintenance nightmare
in the long run.
- Last question is where to trigger the ACD. I made a call-graph that
shows
which functions lead to inet.c function
__connman_inet_modify_address
where the ip address is assigned to the interface. (Dot code below!) Maybe ACD
should be triggered in inet.c function
connman_inet_set_address ?
This also influences vpn-ipconfig.c and vpn-provider.c where
connman_inet_set_address is called. Or should I place it one step upward in
ipconfig.c function
__connman_ipconfig_address_add ?
I didn't look into ACD yet. So my comments might not make sense. So far
the assumption was, whenever ConnMan is not setting a static address it
will ask the dhcp subsystem (unless IPv6 autoconfig) to fetch the
configuration. That's also why ipv4ll lives in this directory. Are you
sure you need to have ACD separated from the current gdhcp code? The RFC
even speaks from a strong relationship to ipv4ll.
Maybe you can start with the current structure and see if it fits or
not. Don't hesitate to post early draft versions of the code if you need
feedback. It is always a bit difficult to give good advice without
looking at real code.
HTH!
Thanks,
Daniel
12:35 p.m.
On Mon 12.02.18 10:58, Daniel Wagner wrote:
Good Morning Christian,
> So, here more concrete questions:
>
> - We could use the utility functions in gdhcp/ipv4ll.c for sending arp packets.
> The functions ipv4ll_send_arp_packet and ipv4ll_arp_socket are not specific to
> dhcp. Should I move them to another folder (src, or new arp) and rename them?
Indeed the ipv4ll_ are not specific to dhcp but currently we just have
one user. If you need it in src, then yes please move it there.
Hi Daniel,
Ok. Then I will move it to src.
> - As you stated, very useful is also gdhcp/client.c. The
functions
> send_probe_packet, send_announce_packet, and so on do exactly what we need for
> ACD. But they are all related to dhcp because they use the structure GDHCPClient.
> ACD should work for dhcp and static ipv4 configurations.
> Do you want some refactoring? Clearly, the simplest way is to copy and rewrite
> these functions and don't touch client.c. What do you think?
Please don't do code duplication because it is a maintenance nightmare
in the long run.
Ok, then refactoring. Good choice!
> - Last question is where to trigger the ACD. I made a call-graph
that shows
> which functions lead to inet.c function
> __connman_inet_modify_address
> where the ip address is assigned to the interface. (Dot code below!) Maybe ACD
> should be triggered in inet.c function
> connman_inet_set_address ?
> This also influences vpn-ipconfig.c and vpn-provider.c where
> connman_inet_set_address is called. Or should I place it one step upward in
> ipconfig.c function
> __connman_ipconfig_address_add ?
I didn't look into ACD yet. So my comments might not make sense. So far
the assumption was, whenever ConnMan is not setting a static address it
will ask the dhcp subsystem (unless IPv6 autoconfig) to fetch the
configuration. That's also why ipv4ll lives in this directory. Are you
sure you need to have ACD separated from the current gdhcp code? The RFC
even speaks from a strong relationship to ipv4ll.
Maybe you can start with the current structure and see if it fits or
not. Don't hesitate to post early draft versions of the code if you need
feedback. It is always a bit difficult to give good advice without
looking at real code.
Ok, I will try to make a proposition. But it has to be more general than only
for dhcp. Also for static ipv4.
HTH!
Thanks,
Daniel
Thanks,
Christian.
12:48 p.m.
Hi Christian,
> Maybe you can start with the current structure and see if it fits
or
> not. Don't hesitate to post early draft versions of the code if you need
> feedback. It is always a bit difficult to give good advice without
> looking at real code.
Ok, I will try to make a proposition. But it has to be more general than only
for dhcp. Also for static ipv4.
Ah, that explains it. Yes, then it makes sense to move the ipv4ll to src.
I should read the RFC :)
Thanks,
Daniel
2:29 p.m.
On Mon 12.02.18 13:48, Daniel Wagner wrote:
Hi Christian,
>> Maybe you can start with the current structure and see if it fits or
>> not. Don't hesitate to post early draft versions of the code if you need
>> feedback. It is always a bit difficult to give good advice without
>> looking at real code.
>
> Ok, I will try to make a proposition. But it has to be more general than only
> for dhcp. Also for static ipv4.
Ah, that explains it. Yes, then it makes sense to move the ipv4ll to src.
Hello Daniel,
what I understood is that ipv4ll is ACD with a random ip address. Further, ACD
has also as fallback to assign an ipv4ll address. And ACD (if enabled) has also
to be done for static ipv4 configurations. So ACD and ipv4ll should be
independent from dhcp.
The problem is that the current implementation mixes ipv4ll with dhcp. The code
is in gdhcp/client.c. All ipv4ll functions use struct GDHCPClient, the enum
GDHCPClientEvent mixes dhcp events with ipv4ll events, and so on.
I see two solutions:
(A) Split up client.c, also GDHCPClient and what ever. The goal would be to
seperate DHCP, ACD, ipv4ll, ARP. Where ipv4ll is a special case of ACD.
DHCP and ACD use ipv4ll as fallback. ARP is a small utility lib which is
used by ACD.
This might look not so bad but the drawback is a big change, lots of testing.
So high risk.
(B) Or I use the current ipv4ll as reference to implement ACD (in include/src),
make a new ipv4ll based on the new ACD which is independent from dhcp.
My plan is to start with (B) and if it works and the code looks good, then maybe
solution (A) becomes closer and easier. So after (B) works we could discuss how
to refactor until we have (A) or something closer to (A).
regards,
Christian.
10:19 a.m.
Hi Christian,
I see two solutions:
(A) Split up client.c, also GDHCPClient and what ever. The goal would be to
seperate DHCP, ACD, ipv4ll, ARP. Where ipv4ll is a special case of ACD.
DHCP and ACD use ipv4ll as fallback. ARP is a small utility lib which is
used by ACD.
This might look not so bad but the drawback is a big change, lots of testing.
So high risk.
(B) Or I use the current ipv4ll as reference to implement ACD (in include/src),
make a new ipv4ll based on the new ACD which is independent from dhcp.
My plan is to start with (B) and if it works and the code looks good, then maybe
solution (A) becomes closer and easier. So after (B) works we could discuss how
to refactor until we have (A) or something closer to (A).
Sounds like a good plan to me. I still want to reach (A) but we start
with (B) and then gradually change the code base to (A).
Thanks,
Daniel
6:29 a.m.
On Wed 14.02.18 11:19, Daniel Wagner wrote:
Hi Christian,
> I see two solutions:
> (A) Split up client.c, also GDHCPClient and what ever. The goal would be to
> seperate DHCP, ACD, ipv4ll, ARP. Where ipv4ll is a special case of ACD.
> DHCP and ACD use ipv4ll as fallback. ARP is a small utility lib which is
> used by ACD.
>
> This might look not so bad but the drawback is a big change, lots of testing.
> So high risk.
> (B) Or I use the current ipv4ll as reference to implement ACD (in include/src),
> make a new ipv4ll based on the new ACD which is independent from dhcp.
>
> My plan is to start with (B) and if it works and the code looks good, then maybe
> solution (A) becomes closer and easier. So after (B) works we could discuss how
> to refactor until we have (A) or something closer to (A).
Sounds like a good plan to me. I still want to reach (A) but we start
with (B) and then gradually change the code base to (A).
Thanks,
Daniel
Hi Daniel,
here are additional notes to the patches sent by pmeerw(a)pmeerw.net. They meet
solution (B). After your review and further improving too meet your expectations,
I will try to change the code towards (A). So, replace IPv4LL in client.c by
calling new ACD.
regards,
Christian.
7:02 a.m.
Good Morning Christian,
On 03/26/2018 08:29 AM, Christian Spielberger wrote:
here are additional notes to the patches sent by pmeerw(a)pmeerw.net.
They meet
solution (B). After your review and further improving too meet your expectations,
I will try to change the code towards (A). So, replace IPv4LL in client.c by
calling new ACD.
Excellent. I started to review the patches yesterday. Unfortunately, I
didn't have enough time to write up all my findings so far. So here just
a quick list of my first impression.
Thanks,
Daniel
1554
days inactive
1601
days old
9 comments
2 participants
participants (2)
-
Christian Spielberger -
Daniel Wagner