Re: [PATCH 1/7] ip-pool: Track IPv4 addresses in use

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2052 bytes --]

Hi Andrew,

>> I mean in
>> theory ip-pool can be reused by netconfig for example, which also tracks IPv4/v6
>> addresses (for some unknown reason, but one thing at a time...)
> 
> That shouldn't be a problem but ok, I'll drop the netdev usage.
> 

netconfig depends on netdev right now, but it really shouldn't.  The only reason 
is that it needs the device name, which really belongs elsewhere since netconfig 
will end up being used by ead, which will not even come with netconfig module at 
all.

So the looser you make this coupling, the easier it will be for us to reuse this 
stuff in other projects.  Start thinking along those lines please.

>>
>>> +
>>> +struct ip_pool_addr4_record {
>>> +     uint32_t addr;
>>> +     uint8_t prefix_len;
>>> +     uint32_t ifindex;
>>> +     bool secondary;
>>> +};
>>
>> Is there a compelling reason to use a dedicated structure (leaking
>> implementation details) instead of returning a newly allocated l_rtnl_address
>> instead?
> 
> Again, convenience, readability, better fit for the use case.  But

readability? Yeah I don't buy that one ;)  You were doing stuff like storing the 
ip address in host order for whatever reason.  And you expect someone reading 
ap.c to remember / know that?

> like I said I can move to using l_rtnl_address, I didn't do it this
> time because it sounded like I convinced you against it.  I'll switch
> to l_rtnl_address though.

I'm fine if you use a custom type inside ip-pool for ease of implementation, but 
you should make the public API 'nice'.

> 
> Using l_rtnl_address here is like using a class for a simple integer
> or boolean value (Java style), I wouldn't call directly accessing the
> actual value "leaking implementation details."

You're returning a const struct foo * back, which basically screams 'I'm sending 
internal details out'.  Also see my comment about host/network ordering.

If you think l_rtnl_address isn't suitable for whatever reason, then lets fix 
that instead.

Regards,
-Denis