[B.A.T.M.A.N.] [PATCH] batman-adv: move smallest_signed_int(), seq_before() and seq_after() into main.h

[B.A.T.M.A.N.] [PATCH] batman-adv: move smallest_signed_int(), seq_before() and seq_after() into main.h

[Antonio Quartulli]

smallest_signed_int(), seq_before() and seq_after() are very useful
functions that help to handle comparisons between sequence numbers.
However they were only defined in vis.c. With this patch every
batman-adv function will be able to use them.

Signed-off-by: Antonio Quartulli <ordex@autistici.org>
---
 main.h |   16 ++++++++++++++++
 vis.c  |   16 ----------------
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/main.h b/main.h
index 3ca3941..90db244 100644
--- a/main.h
+++ b/main.h
@@ -181,4 +181,20 @@ static inline int compare_eth(void *data1, void *data2)
 
 #define atomic_dec_not_zero(v)	atomic_add_unless((v), -1, 0)
 
+/* Returns the smallest signed integer in two's complement with the sizeof x */
+#define smallest_signed_int(x) (1u << (7u + 8u * (sizeof(x) - 1u)))
+
+/* Checks if a sequence number x is a predecessor/successor of y.
+ * they handle overflows/underflows and can correctly check for a
+ * predecessor/successor unless the variable sequence number has grown by
+ * more then 2**(bitwidth(x)-1)-1.
+ * This means that for a uint8_t with the maximum value 255, it would think:
+ *  - when adding nothing - it is neither a predecessor nor a successor
+ *  - before adding more than 127 to the starting value - it is a predecessor,
+ *  - when adding 128 - it is neither a predecessor nor a successor,
+ *  - after adding more than 127 to the starting value - it is a successor */
+#define seq_before(x, y) ({typeof(x) _dummy = (x - y); \
+			_dummy > smallest_signed_int(_dummy); })
+#define seq_after(x, y) seq_before(y, x)
+
 #endif /* _NET_BATMAN_ADV_MAIN_H_ */
diff --git a/vis.c b/vis.c
index c39f20c..e1ed552 100644
--- a/vis.c
+++ b/vis.c
@@ -30,22 +30,6 @@
 
 #define MAX_VIS_PACKET_SIZE 1000
 
-/* Returns the smallest signed integer in two's complement with the sizeof x */
-#define smallest_signed_int(x) (1u << (7u + 8u * (sizeof(x) - 1u)))
-
-/* Checks if a sequence number x is a predecessor/successor of y.
- * they handle overflows/underflows and can correctly check for a
- * predecessor/successor unless the variable sequence number has grown by
- * more then 2**(bitwidth(x)-1)-1.
- * This means that for a uint8_t with the maximum value 255, it would think:
- *  - when adding nothing - it is neither a predecessor nor a successor
- *  - before adding more than 127 to the starting value - it is a predecessor,
- *  - when adding 128 - it is neither a predecessor nor a successor,
- *  - after adding more than 127 to the starting value - it is a successor */
-#define seq_before(x, y) ({typeof(x) _dummy = (x - y); \
-			_dummy > smallest_signed_int(_dummy); })
-#define seq_after(x, y) seq_before(y, x)
-
 static void start_vis_timer(struct bat_priv *bat_priv);
 
 /* free the info */
-- 
1.7.3.4

[B.A.T.M.A.N.] [RFC/PATCH] net: add seq_before/seq_after functions

[Antonio Quartulli]

Introduce two operations to handle comparison between packet sequence
numbers taking into account overflow/wraparound. Batman-adv uses
these functions already to check for successor packet even in case of
overflow.
---

I added this two functions in net.h because I didn't really know where
best placement is. I saw several modules that redefine their own functions
for the same purpose.

 include/linux/net.h |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/include/linux/net.h b/include/linux/net.h
index 94de83c..c7bc9bf 100644
--- a/include/linux/net.h
+++ b/include/linux/net.h
@@ -295,4 +295,21 @@ extern struct ratelimit_state net_ratelimit_state;
 #endif
 
 #endif /* __KERNEL__ */
+
+/* Returns the smallest signed integer in two's complement with the sizeof x */
+#define smallest_signed_int(x) (1u << (7u + 8u * (sizeof(x) - 1u)))
+
+/* Checks if a sequence number x is a predecessor/successor of y.
+ * they handle overflows/underflows and can correctly check for a
+ * predecessor/successor unless the variable sequence number has grown by
+ * more then 2**(bitwidth(x)-1)-1.
+ * This means that for a uint8_t with the maximum value 255, it would think:
+ *  - when adding nothing - it is neither a predecessor nor a successor
+ *  - before adding more than 127 to the starting value - it is a predecessor,
+ *  - when adding 128 - it is neither a predecessor nor a successor,
+ *  - after adding more than 127 to the starting value - it is a successor */
+#define seq_before(x, y) ({typeof(x) _dummy = (x - y); \
+			_dummy > smallest_signed_int(_dummy); })
+#define seq_after(x, y) seq_before(y, x)
+
 #endif	/* _LINUX_NET_H */
-- 
1.7.3.4

Re: [B.A.T.M.A.N.] [RFC/PATCH] net: add seq_before/seq_after functions

[Sven Eckelmann]

[-- Attachment #1: Type: Text/Plain, Size: 3050 bytes --]

On Wednesday 18 May 2011 14:22:13 Antonio Quartulli wrote:
> Introduce two operations to handle comparison between packet sequence
> numbers taking into account overflow/wraparound. Batman-adv uses
> these functions already to check for successor packet even in case of
> overflow.
> ---
> 
> I added this two functions in net.h because I didn't really know where
> best placement is. I saw several modules that redefine their own functions
> for the same purpose.

Wouldn't it be better to ask linux-kernel what Linus/David/... think
about the idea to provide the functionality of seq_after and seq_before
seen at net/batman-adv/vis.c. They will tell you that this stuff
has to be placed in include/linux/kernel.h and let you know that it is a
complete stupid implementation because it never checks that the type is
the same. Then you will start to reimplement it the following way:


/* Returns the smallest signed integer in two's complement with the sizeof x */
#define smallest_signed_int(x) (1u << (7u + 8u * (sizeof(x) - 1u)))

/* Checks if a sequence number x is a predecessor/successor of y.
 * they handle overflows/underflows and can correctly check for a
 * predecessor/successor unless the variable sequence number has grown by
 * more then 2**(bitwidth(x)-1)-1.
 * This means that for a uint8_t with the maximum value 255, it would think:
 *  - when adding nothing - it is neither a predecessor nor a successor
 *  - before adding more than 127 to the starting value - it is a predecessor,
 *  - when adding 128 - it is neither a predecessor nor a successor,
 *  - after adding more than 127 to the starting value - it is a successor */
#define seq_before(x, y) ({typeof(x) _d1 = (x); \
			{typeof(y) _d2 = (y); \
			(void) (&_d1 == &_d2); \
			(_d1 - _d2) > smallest_signed_int(_d1); })
#define seq_after(x, y) seq_before(y, x)


Or you could just sent this version as inline code and ask if it should
be moved from net/batman-adv/vis.c to a kernel header like
include/linux/kernel.h to provide a somewhat general solution for the
seq_before/seq_after. And maybe suggest the use in other protocols.
Hopefully they will say something like "no" or "yes, place it there".
Independent from the answer, we have a reference which clearly protects
our actions (your patch(es)). But somebody will still scream :)

Just look at Documentation/development-process/3.Early-stage
"3.3: WHO DO YOU TALK TO?" to find more about that "process".

Btw. I never tested the changes which does the type comparison. And
propably the ppp guys will scream because you conflict with there
definition of seq_before and seq_after - so you would have to remove
that definition from both places, get acks from both parties by proving
that everything still works the same way and get it merged by someone.
But providing a patch is propably the second step after getting the the
ok by a main kernel maintainer.

... and I will now write my own daily soap about people on mailinglist.

Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: move smallest_signed_int(), seq_before() and seq_after() into main.h

[Sven Eckelmann]

[-- Attachment #1: Type: Text/Plain, Size: 3234 bytes --]

Antonio Quartulli wrote:
> smallest_signed_int(), seq_before() and seq_after() are very useful
> functions that help to handle comparisons between sequence numbers.
> However they were only defined in vis.c. With this patch every
> batman-adv function will be able to use them.
> 
> Signed-off-by: Antonio Quartulli <ordex@autistici.org>


Acked-by: Sven Eckelmann <sven@narfation.org>

David S. Miller doesn't want to have the seq_before/seq_after in a global 
header: http://lkml.org/lkml/2011/5/19/88

I would still like to propose the type check on top of that patch

Kind regards,
	Sven

> ---
>  main.h |   16 ++++++++++++++++
>  vis.c  |   16 ----------------
>  2 files changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/main.h b/main.h
> index 3ca3941..90db244 100644
> --- a/main.h
> +++ b/main.h
> @@ -181,4 +181,20 @@ static inline int compare_eth(void *data1, void
> *data2)
> 
>  #define atomic_dec_not_zero(v)	atomic_add_unless((v), -1, 0)
> 
> +/* Returns the smallest signed integer in two's complement with the sizeof
> x */ +#define smallest_signed_int(x) (1u << (7u + 8u * (sizeof(x) - 1u)))
> +
> +/* Checks if a sequence number x is a predecessor/successor of y.
> + * they handle overflows/underflows and can correctly check for a
> + * predecessor/successor unless the variable sequence number has grown by
> + * more then 2**(bitwidth(x)-1)-1.
> + * This means that for a uint8_t with the maximum value 255, it would
> think: + *  - when adding nothing - it is neither a predecessor nor a
> successor + *  - before adding more than 127 to the starting value - it is
> a predecessor, + *  - when adding 128 - it is neither a predecessor nor a
> successor, + *  - after adding more than 127 to the starting value - it is
> a successor */ +#define seq_before(x, y) ({typeof(x) _dummy = (x - y); \
> +			_dummy > smallest_signed_int(_dummy); })
> +#define seq_after(x, y) seq_before(y, x)
> +
>  #endif /* _NET_BATMAN_ADV_MAIN_H_ */
> diff --git a/vis.c b/vis.c
> index c39f20c..e1ed552 100644
> --- a/vis.c
> +++ b/vis.c
> @@ -30,22 +30,6 @@
> 
>  #define MAX_VIS_PACKET_SIZE 1000
> 
> -/* Returns the smallest signed integer in two's complement with the sizeof
> x */ -#define smallest_signed_int(x) (1u << (7u + 8u * (sizeof(x) - 1u)))
> -
> -/* Checks if a sequence number x is a predecessor/successor of y.
> - * they handle overflows/underflows and can correctly check for a
> - * predecessor/successor unless the variable sequence number has grown by
> - * more then 2**(bitwidth(x)-1)-1.
> - * This means that for a uint8_t with the maximum value 255, it would
> think: - *  - when adding nothing - it is neither a predecessor nor a
> successor - *  - before adding more than 127 to the starting value - it is
> a predecessor, - *  - when adding 128 - it is neither a predecessor nor a
> successor, - *  - after adding more than 127 to the starting value - it is
> a successor */ -#define seq_before(x, y) ({typeof(x) _dummy = (x - y); \
> -			_dummy > smallest_signed_int(_dummy); })
> -#define seq_after(x, y) seq_before(y, x)
> -
>  static void start_vis_timer(struct bat_priv *bat_priv);
> 
>  /* free the info */

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[B.A.T.M.A.N.] [PATCH] batman-adv: Check type of x and y in seq_(before|after)

[Sven Eckelmann]

seq_before and seq_after depend on the fact that both sequence numbers
have the same type and thus the same bitwidth. We can ensure that by
compile time checking using a compare between the pointer to the
temporary buffers which were created using the typeof of both
parameters. For example gcc would create a warning like
"warning: comparison of distinct pointer types lacks a cast".

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 main.h |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/main.h b/main.h
index db29444..3395bf9 100644
--- a/main.h
+++ b/main.h
@@ -196,8 +196,11 @@ static inline int compare_eth(const void *data1, const void *data2)
  *  - before adding more than 127 to the starting value - it is a predecessor,
  *  - when adding 128 - it is neither a predecessor nor a successor,
  *  - after adding more than 127 to the starting value - it is a successor */
-#define seq_before(x, y) ({typeof(x) _dummy = (x - y); \
-			_dummy > smallest_signed_int(_dummy); })
+#define seq_before(x, y) ({typeof(x) _d1 = (x); \
+			  typeof(y) _d2 = (y); \
+			  typeof(x) _dummy = (_d1 - _d2); \
+			  (void) (&_d1 == &_d2); \
+			  _dummy > smallest_signed_int(_dummy); })
 #define seq_after(x, y) seq_before(y, x)
 
 #endif /* _NET_BATMAN_ADV_MAIN_H_ */
-- 
1.7.5.1

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: move smallest_signed_int(), seq_before() and seq_after() into main.h

[Marek Lindner]

On Wednesday 18 May 2011 09:20:50 Antonio Quartulli wrote:
> smallest_signed_int(), seq_before() and seq_after() are very useful
> functions that help to handle comparisons between sequence numbers.
> However they were only defined in vis.c. With this patch every
> batman-adv function will be able to use them.

Applied in revision 7962ae7.

Thanks,
Marek

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: Check type of x and y in seq_(before|after)

[Marek Lindner]

On Thursday 19 May 2011 21:43:08 Sven Eckelmann wrote:
> seq_before and seq_after depend on the fact that both sequence numbers
> have the same type and thus the same bitwidth. We can ensure that by
> compile time checking using a compare between the pointer to the
> temporary buffers which were created using the typeof of both
> parameters. For example gcc would create a warning like
> "warning: comparison of distinct pointer types lacks a cast".

Applied in revision 288421e.

Thanks,
Marek