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: Planning Address Conflict Detection RFC-5227
(Christian Spielberger)
2. Re: Planning Address Conflict Detection RFC-5227 (Daniel Wagner)
3. Re: [RFC PATCH 00/27] Implement Address Conflict Detection
(ACD), RFC 5227 (Daniel Wagner)
4. Re: [RFC PATCH 00/27] Implement Address Conflict Detection
(ACD), RFC 5227 (Christian Spielberger)
5. Re: [RFC PATCH 00/27] Implement Address Conflict Detection
(ACD), RFC 5227 (Daniel Wagner)
----------------------------------------------------------------------
Message: 1
Date: Mon, 26 Mar 2018 08:29:10 +0200
From: Christian Spielberger <[email protected]>
To: Daniel Wagner <[email protected]>
Cc: [email protected]
Subject: Re: Planning Address Conflict Detection RFC-5227
Message-ID: <[email protected]>
Content-Type: text/plain; charset=us-ascii
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 [email protected]. 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.
------------------------------
Message: 2
Date: Mon, 26 Mar 2018 09:02:49 +0200
From: Daniel Wagner <[email protected]>
To: Christian Spielberger <[email protected]>
Cc: [email protected]
Subject: Re: Planning Address Conflict Detection RFC-5227
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8
Good Morning Christian,
On 03/26/2018 08:29 AM, Christian Spielberger wrote:
> here are additional notes to the patches sent by [email protected]. 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
------------------------------
Message: 3
Date: Mon, 26 Mar 2018 09:18:01 +0200
From: Daniel Wagner <[email protected]>
To: Peter Meerwald-Stadler <[email protected]>,
[email protected]
Cc: [email protected]
Subject: Re: [RFC PATCH 00/27] Implement Address Conflict Detection
(ACD), RFC 5227
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8
Good Morning Peter and Christian,
On 03/21/2018 02:42 PM, Peter Meerwald-Stadler wrote:
> Patch series implementing Address Conflict Detection (ACD) according to RFC
> 5227,
> please comment/review! The patches are served in small chunks for review.
>
> ACD is not enabled by default, but can be enabled in Connman's main.conf.
>
> Patches 1 to 8 are unrelated and move common code to src/shared.
.gitignore: So far we have not added any of the typical vim/emacs/...
save files to the list. It's something you can configure in our editor
settings not to drop does files everywhere. For example I have
(setq backup-directory-alist `(("." . "~/.saves")))
in my .emacs file. No more stupid filename~ everywhere and still have my
backups.
Next build every patch standalone. I found some of them not compiling.
Also use 'make distcheck'. Then you will see that you need to change
#include "shared/acd.h"
to
#include "src/shared/acd.h"
> Patch 9 adds new files for ACD and the following patches add functionality.
acd: add struct _ACDHost
We don't to C++ structure names patterns (camel casing). Maybe
struct acd_host
?
service: init and start of acd
service: add acd callback functions
service: add ipv4ll as fallback for acd
I am a bit reluctant to have this logic in service.c. The file is
already too big and has a lot of high level logic in it. Furthermore, I
would have expected to find this stuff in ipconfig or maybe in network.
I haven't spend a lot of time looking into the details. Sorry about
that. But I wanted to give a first quick feedback so you can start
fixing up the obvious stuff. Overall the patches do look good.
Maybe adding a bit more commit text wouldn't hurt. I rather have a good
commit message than random comments in the code. Comments bit rot really.
Thanks,
Daniel
------------------------------
Message: 4
Date: Mon, 26 Mar 2018 09:35:48 +0200
From: Christian Spielberger <[email protected]>
To: Daniel Wagner <[email protected]>
Cc: Peter Meerwald-Stadler <[email protected]>, [email protected]
Subject: Re: [RFC PATCH 00/27] Implement Address Conflict Detection
(ACD), RFC 5227
Message-ID: <[email protected]>
Content-Type: text/plain; charset=us-ascii
On Mon 26.03.18 09:18, Daniel Wagner wrote:
> Good Morning Peter and Christian,
Hi Daniel,
you will find my answers inline.
>
> On 03/21/2018 02:42 PM, Peter Meerwald-Stadler wrote:
> > Patch series implementing Address Conflict Detection (ACD) according to RFC
> > 5227,
> > please comment/review! The patches are served in small chunks for review.
> >
> > ACD is not enabled by default, but can be enabled in Connman's main.conf.
> >
> > Patches 1 to 8 are unrelated and move common code to src/shared.
>
> .gitignore: So far we have not added any of the typical vim/emacs/...
> save files to the list. It's something you can configure in our editor
> settings not to drop does files everywhere. For example I have
>
> (setq backup-directory-alist `(("." . "~/.saves")))
>
> in my .emacs file. No more stupid filename~ everywhere and still have my
> backups.
Ok. Will drop this patch.
>
> Next build every patch standalone. I found some of them not compiling.
> Also use 'make distcheck'. Then you will see that you need to change
>
> #include "shared/acd.h"
>
> to
>
> #include "src/shared/acd.h"
>
Ok. Will do this.
> > Patch 9 adds new files for ACD and the following patches add functionality.
>
> acd: add struct _ACDHost
>
> We don't to C++ structure names patterns (camel casing). Maybe
>
> struct acd_host
>
> ?
Right. I took the naming scheme from gdhcp dir. The code in src looks different.
I will change to acd_host.
>
> service: init and start of acd
> service: add acd callback functions
> service: add ipv4ll as fallback for acd
>
> I am a bit reluctant to have this logic in service.c. The file is
> already too big and has a lot of high level logic in it. Furthermore, I
> would have expected to find this stuff in ipconfig or maybe in network.
>
Right. The dhcp callback dhcp_success(...) and the set_connected_manual(..) are
already in network where acd should be initiated. So maybe this is the best
place?
> I haven't spend a lot of time looking into the details. Sorry about
> that. But I wanted to give a first quick feedback so you can start
> fixing up the obvious stuff. Overall the patches do look good.
Ok. As you state above.
>
> Maybe adding a bit more commit text wouldn't hurt. I rather have a good
> commit message than random comments in the code. Comments bit rot really.
Also, ok. Will write longer commit text.
>
> Thanks,
> Daniel
Regards,
Chris.
------------------------------
Message: 5
Date: Mon, 26 Mar 2018 10:34:12 +0200
From: Daniel Wagner <[email protected]>
To: Christian Spielberger <[email protected]>
Cc: Peter Meerwald-Stadler <[email protected]>, [email protected]
Subject: Re: [RFC PATCH 00/27] Implement Address Conflict Detection
(ACD), RFC 5227
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8
>>> Patch 9 adds new files for ACD and the following patches add functionality.
>>
>> acd: add struct _ACDHost
>>
>> We don't to C++ structure names patterns (camel casing). Maybe
>>
>> struct acd_host
>>
>> ?
>
> Right. I took the naming scheme from gdhcp dir. The code in src looks
> different.
> I will change to acd_host.
The gdhcp is an odd place. The master plan is to steal the libdhcp from
systemd back one day. The systemd code is based on our gdhcp and was
also written by Patrik. So don't look too close on it :)
>> service: init and start of acd
>> service: add acd callback functions
>> service: add ipv4ll as fallback for acd
>>
>> I am a bit reluctant to have this logic in service.c. The file is
>> already too big and has a lot of high level logic in it. Furthermore, I
>> would have expected to find this stuff in ipconfig or maybe in network.
>>
>
> Right. The dhcp callback dhcp_success(...) and the set_connected_manual(..)
> are
> already in network where acd should be initiated. So maybe this is the best
> place?
network.c sounds also much better then service.c
>> Maybe adding a bit more commit text wouldn't hurt. I rather have a good
>> commit message than random comments in the code. Comments bit rot really.
>
> Also, ok. Will write longer commit text.
Excellent. Thanks a lot!
Daniel
------------------------------
Subject: Digest Footer
_______________________________________________
connman mailing list
[email protected]
https://lists.01.org/mailman/listinfo/connman
------------------------------
End of connman Digest, Vol 29, Issue 27
***************************************
