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

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Andrew,

On 5/25/21 7:55 AM, Andrew Zaborowski wrote:
> Add the ip-pool submodule that tracks IPv4 addresses in use on the
> system for use when selecting the address for a new AP.
> ---
>   Makefile.am   |   1 +
>   src/ip-pool.c | 160 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   src/ip-pool.h |  32 ++++++++++
>   3 files changed, 193 insertions(+)
>   create mode 100644 src/ip-pool.c
>   create mode 100644 src/ip-pool.h
> 

<snip>

So I'm completely fine with the implementation now...

> diff --git a/src/ip-pool.h b/src/ip-pool.h
> new file mode 100644
> index 00000000..6a220735
> --- /dev/null
> +++ b/src/ip-pool.h
> @@ -0,0 +1,32 @@
> +/*
> + *
> + *  Wireless daemon for Linux
> + *
> + *  Copyright (C) 2021  Intel Corporation. All rights reserved.
> + *
> + *  This library is free software; you can redistribute it and/or
> + *  modify it under the terms of the GNU Lesser General Public
> + *  License as published by the Free Software Foundation; either
> + *  version 2.1 of the License, or (at your option) any later version.
> + *
> + *  This library 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
> + *  Lesser General Public License for more details.
> + *
> + *  You should have received a copy of the GNU Lesser General Public
> + *  License along with this library; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + */
> +
> +struct netdev;

Is there a strong reason for making this tighly-coupled to netdev?  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...)

> +
> +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?

> +
> +const struct ip_pool_addr4_record *ip_pool_get_addr4(struct netdev *netdev);
> 

This function is used exactly once, when the profile is loaded.  So it isn't 
exactly on the hot-path where raw access to the store is needed.

Regards,
-Denis