Re: [PATCH 01/13] net: Add l_net_clear_host_bits utility

Re: [PATCH 01/13] net: Add l_net_clear_host_bits utility

[Andrew Zaborowski]

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

On Fri, 13 May 2022 at 16:15, Denis Kenzior <denkenz(a)gmail.com> wrote:
> >>> +static inline void l_net_clear_host_bits(uint8_t *address, uint8_t prefix_len,
> >>> +                                             uint8_t bytes)
> >>
> >> I'm not super excited about the naming.  What about l_net_prefix_from_address()?
> >
> > Ok, I guess host mask is an IPv4 term with the v6 term being interface
> > ID.  l_net_prefix_from_address sounds good, or how about
>
> You could also add a separate v4/v6 version instead?  Your set only uses this
> for v6 right now, and you'd be able to drop the bytes parameter?  You could also
> use a static buffer for the return value (or simply a uint32 for the v4 version)
> and make address in-only instead of in-out.

Ok.

>
> > l_net_mask_prefix?
>
> The use of l_net_prefix_from_address() would make it very clear what is
> happening in netconfig_add_v6_static_address_routes().  l_net_mask_prefix better
> suites the usage in patch 13, though your original naming also works well here.
>
> Question is, if you're already sanitizing & dropping the address portion in
> netconfig_add_v6_static_routes(), do you even need the use of
> l_net_clear_host_bits in patch 13?

netconfig_add_v6_static_routes() is only used for the static configs.
In icmp.c I use it for the RA data and I think it's nice if the higher
layer (netconfig) can trust that what it received from the ICMPv6
layer is already validated.

>
> Anyhow, maybe the outcome from this discussion is to just put this into
> net-private.h for now and we can agree on the name later if this function needs
> to be exposed into public API.

Ok.

Best regards

Re: [PATCH 01/13] net: Add l_net_clear_host_bits utility

[Denis Kenzior]

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

Hi Andrew,

>>> +static inline void l_net_clear_host_bits(uint8_t *address, uint8_t prefix_len,
>>> +                                             uint8_t bytes)
>>
>> I'm not super excited about the naming.  What about l_net_prefix_from_address()?
> 
> Ok, I guess host mask is an IPv4 term with the v6 term being interface
> ID.  l_net_prefix_from_address sounds good, or how about

You could also add a separate v4/v6 version instead?  Your set only uses this 
for v6 right now, and you'd be able to drop the bytes parameter?  You could also 
use a static buffer for the return value (or simply a uint32 for the v4 version) 
and make address in-only instead of in-out.

> l_net_mask_prefix?

The use of l_net_prefix_from_address() would make it very clear what is 
happening in netconfig_add_v6_static_address_routes().  l_net_mask_prefix better 
suites the usage in patch 13, though your original naming also works well here.

Question is, if you're already sanitizing & dropping the address portion in 
netconfig_add_v6_static_routes(), do you even need the use of 
l_net_clear_host_bits in patch 13?

Anyhow, maybe the outcome from this discussion is to just put this into 
net-private.h for now and we can agree on the name later if this function needs 
to be exposed into public API.

Regards,
-Denis

Re: [PATCH 01/13] net: Add l_net_clear_host_bits utility

[Andrew Zaborowski]

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

Hi Denis,

On Thu, 12 May 2022 at 18:02, Denis Kenzior <denkenz(a)gmail.com> wrote:
> On 5/5/22 18:15, Andrew Zaborowski wrote:
> > Add function that zeros bits after prefix_len'th bit in an address.  Put
> > it in net.h so it can be use by both netconfig.c and icmp6.c,
> > potentially others.
> > ---
> >   ell/net.h | 13 +++++++++++++
> >   1 file changed, 13 insertions(+)
> >
> > diff --git a/ell/net.h b/ell/net.h
> > index 0521d75..e193eef 100644
> > --- a/ell/net.h
> > +++ b/ell/net.h
> > @@ -61,6 +61,19 @@ static inline bool l_net_prefix_matches(const void *a, const void *b,
> >       return !bits || ((left ^ right) & (0xff00u >> bits)) == 0;
> >   }
> >
> > +static inline void l_net_clear_host_bits(uint8_t *address, uint8_t prefix_len,
> > +                                             uint8_t bytes)
>
> I'm not super excited about the naming.  What about l_net_prefix_from_address()?

Ok, I guess host mask is an IPv4 term with the v6 term being interface
ID.  l_net_prefix_from_address sounds good, or how about
l_net_mask_prefix?

Best regards

Re: [PATCH 01/13] net: Add l_net_clear_host_bits utility

[Denis Kenzior]

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

Hi Andrew,

On 5/5/22 18:15, Andrew Zaborowski wrote:
> Add function that zeros bits after prefix_len'th bit in an address.  Put
> it in net.h so it can be use by both netconfig.c and icmp6.c,
> potentially others.
> ---
>   ell/net.h | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/ell/net.h b/ell/net.h
> index 0521d75..e193eef 100644
> --- a/ell/net.h
> +++ b/ell/net.h
> @@ -61,6 +61,19 @@ static inline bool l_net_prefix_matches(const void *a, const void *b,
>   	return !bits || ((left ^ right) & (0xff00u >> bits)) == 0;
>   }
>   
> +static inline void l_net_clear_host_bits(uint8_t *address, uint8_t prefix_len,
> +						uint8_t bytes)

I'm not super excited about the naming.  What about l_net_prefix_from_address()? 
  Alternatively, we do have net-private.h where we can stuff this for now and 
not expose it to public API.

> +{
> +	uint8_t last_byte = prefix_len / 8;
> +
> +	if (prefix_len & 7) {
> +		address[last_byte] &= 0xff00 >> (prefix_len & 7);
> +		last_byte++;
> +	}
> +
> +	memset(address + last_byte, 0, bytes - last_byte);
> +}
> +
>   #ifdef __cplusplus
>   }
>   #endif
> 

Regards,
-Denis

[PATCH 01/13] net: Add l_net_clear_host_bits utility

[Andrew Zaborowski]

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

Add function that zeros bits after prefix_len'th bit in an address.  Put
it in net.h so it can be use by both netconfig.c and icmp6.c,
potentially others.
---
 ell/net.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/ell/net.h b/ell/net.h
index 0521d75..e193eef 100644
--- a/ell/net.h
+++ b/ell/net.h
@@ -61,6 +61,19 @@ static inline bool l_net_prefix_matches(const void *a, const void *b,
 	return !bits || ((left ^ right) & (0xff00u >> bits)) == 0;
 }
 
+static inline void l_net_clear_host_bits(uint8_t *address, uint8_t prefix_len,
+						uint8_t bytes)
+{
+	uint8_t last_byte = prefix_len / 8;
+
+	if (prefix_len & 7) {
+		address[last_byte] &= 0xff00 >> (prefix_len & 7);
+		last_byte++;
+	}
+
+	memset(address + last_byte, 0, bytes - last_byte);
+}
+
 #ifdef __cplusplus
 }
 #endif
-- 
2.32.0