Hi Tomasz,
On 03/08/2013 09:32 AM, Tomasz Bursztyka wrote:
I really should spend more time in explaining stuff in the cover letter.
These patches are kind of left overs from the bug fixing session and
not really necessary if you think in 'does it fix a bug' or 'it's a
new feature'. These patches are just plain maintaining work which
improve the structure of the code and should help to the next reader
not to spend weeks going over the code again and again which I did.
It's like that in many parts in connman, and the code has not much to do
here: usually it's because the domain it's touching is really complex.
And the coder will have to make effort to understand the domain to
understand the code.
In iptables.c the complexity comes from xtables, any rewrite won't fix
it.
There is complexity coming from xtables but that doesn't mean we need to
have complex code as well.
And you know that.
Sure I know it, but I do not stop trying to improve things just because
there are plans to abandon iptables.
Talking about next reader in this specific context is not really an
argument.
You do not know when iptables v2 or nftables will be around. Also, it is
very likely that ConnMan runs on a LTSI kernel which will never see
iptables v2 or nftables.
As long we don't have one of those new APIs, iptables v1 is the one API
we need to support. If you don't want to maintain it, there is still me
around for this.
So long we have this code in ConnMan, there is a next reader.
In most of your rewrite in later patches, the comments your added are
the real worthy part, not the code rewrite itself.
I really think those comments go hand in hand with the refactoring. And
as I say it is a refactoring, no feature hub or bug fix. Maintains work,
cleaning up, removing duplicated code etc.
I understand you spend much time on iptables.c to understand it, I went
through the same hassle. But refactoring is a trap, especially there.
Moreover it can introduce regressions.
And that is why I spend time getting tests up and running. All the
changes will not break the tests. Currently, the only usage for iptables
in ConnMan is to setup the NAT. There a couple of tests which verify
that this use case still works.
cheers,
daniel
_______________________________________________
connman mailing list
[email protected]
http://lists.connman.net/listinfo/connman
