diff mbox

remove unnecessary logspam

Message ID 1401911616-18318-1-git-send-email-gaul@web-yard.de
State Superseded, archived
Headers show

Commit Message

André Gaul June 4, 2014, 7:53 p.m. UTC
This patch removes unnecessary logspam which resulted from superfluous
calls to net_ratelimit(). With the supplied patch, net_ratelimit() is
called after the loglevel has been checked.

Signed-off-by: André Gaul <gaul@web-yard.de>
---
 main.h    | 16 ++++++++++++----
 routing.c |  4 ++--
 2 files changed, 14 insertions(+), 6 deletions(-)

Comments

Antonio Quartulli June 9, 2014, 8:54 a.m. UTC | #1
Hi André,

On 04/06/14 21:53, André Gaul wrote:
> This patch removes unnecessary logspam which resulted from superfluous
> calls to net_ratelimit(). With the supplied patch, net_ratelimit() is
> called after the loglevel has been checked.

good catch!

> -#define batadv_dbg(type, bat_priv, fmt, arg...)			\
> +/* possibly ratelimited debug output */
> +#define _batadv_dbg(type, bat_priv, ratelimited, fmt, arg...)			\

can you please put the ending "\" at the same position of the ones below
? (This way the entire macro definition looks better).

>  	do {							\
> -		if (atomic_read(&bat_priv->log_level) & type)	\
> +		if (atomic_read(&bat_priv->log_level) & type && \
> +		    (!ratelimited || net_ratelimit()))	\

same here

>  			batadv_debug_log(bat_priv, fmt, ## arg);\
>  	}							\
>  	while (0)
>  #else /* !CONFIG_BATMAN_ADV_DEBUG */
> -__printf(3, 4)
> -static inline void batadv_dbg(int type __always_unused,
> +__printf(4, 5)
> +static inline void _batadv_dbg(int type __always_unused,
>  			      struct batadv_priv *bat_priv __always_unused,
> +			      int ratelimited __always_unused,
>  			      const char *fmt __always_unused, ...)
>  {
>  }
>  #endif
>  
> +#define batadv_dbg(type, bat_priv, arg...) \
> +	_batadv_dbg(type, bat_priv, 0, ## arg)
> +#define batadv_dbg_ratelimited(type, bat_priv, arg...) \
> +	_batadv_dbg(type, bat_priv, 1, ## arg)
> +
>  #define batadv_info(net_dev, fmt, arg...)				\
>  	do {								\
>  		struct net_device *_netdev = (net_dev);                 \
> diff --git a/routing.c b/routing.c
> index 3514153..c679c6f 100644
> --- a/routing.c
> +++ b/routing.c
> @@ -706,7 +706,7 @@ static int batadv_check_unicast_ttvn(struct batadv_priv *bat_priv,
>  	if (batadv_tt_local_client_is_roaming(bat_priv, ethhdr->h_dest, vid)) {
>  		if (batadv_reroute_unicast_packet(bat_priv, unicast_packet,
>  						  ethhdr->h_dest, vid))
> -			net_ratelimited_function(batadv_dbg, BATADV_DBG_TT,
> +			batadv_dbg_ratelimited(BATADV_DBG_TT,
>  						 bat_priv,
>  						 "Rerouting unicast packet to %pM (dst=%pM): Local Roaming\n",
>  						 unicast_packet->dest,

The arguments on a new line must be lined up to the first column after
the opening parenthesis. (I think that checkpatch would have suggested
the same)

> @@ -752,7 +752,7 @@ static int batadv_check_unicast_ttvn(struct batadv_priv *bat_priv,
>  	 */
>  	if (batadv_reroute_unicast_packet(bat_priv, unicast_packet,
>  					  ethhdr->h_dest, vid)) {
> -		net_ratelimited_function(batadv_dbg, BATADV_DBG_TT, bat_priv,
> +		batadv_dbg_ratelimited(BATADV_DBG_TT, bat_priv,
>  					 "Rerouting unicast packet to %pM (dst=%pM): TTVN mismatch old_ttvn=%u new_ttvn=%u\n",
>  					 unicast_packet->dest, ethhdr->h_dest,
>  					 old_ttvn, curr_ttvn);

same here.


The rest looks good!
Marek Lindner June 14, 2014, 1:44 a.m. UTC | #2
On Tuesday 10 June 2014 17:45:17 André Gaul wrote:
> btw: do you have any pointers to documentation that can help me
> understand why the kernel developers still use email as the primary
> channel for submitting patches? Given the fact that git gracefully
> handles the transmission of code, the handling of all code via email
> appears archaic and too complicated to me. (no flame war intended, I
> just want to learn about the reasons!) ;)

Traditionally, patches on the mailing list make it easy to review & comment 
patches by everyone.

What is the alternative you have in mind (no vendor lock-in please) ?

Cheers,
Marek
André Gaul June 15, 2014, 12:32 p.m. UTC | #3
Hi Marek,

Am 14.06.2014 03:44, schrieb Marek Lindner:
> Traditionally, patches on the mailing list make it easy to review & comment 
> patches by everyone.
> 
> What is the alternative you have in mind (no vendor lock-in please) ?

I got used to simple webuis for reviews+comments à la github or
bitbucket. I can fully understand that kernel developers do not want to
depend on a company such as github. The only free software alternative
that comes to my mind is gitlab but I don't have any experience with
gitlab and big projects.

Patches on the mailing list make it easy for people who already do so
for some time. E.g., if you're not on the list, then it's not that easy
to apply a submitted patch (you'll have to copy+paste the patch). Also
the context of a patch is missing in the mail: you don't see the full
patched version and every potential reviewer has to apply the patch
her/himself, thus collectively wasting time for a task that can be fully
automated. Furthermore, we've just seen in this thread how vulnerable
the procedure is to misbehaving mail clients. ;) This also wasted some
of my and Antonio's time.

As said before: I just wanted to know about the reasons -- refusing
vendor lock-ins certainly is a reason and I can agree on trading a bit
of comfort for independence. :)

Cheers and keep up the good work on B.A.T.M.A.N.,
André
diff mbox

Patch

diff --git a/main.h b/main.h
index 87e7196..df57639 100644
--- a/main.h
+++ b/main.h
@@ -239,21 +239,29 @@  enum batadv_dbg_level {
 int batadv_debug_log(struct batadv_priv *bat_priv, const char *fmt, ...)
 __printf(2, 3);
 
-#define batadv_dbg(type, bat_priv, fmt, arg...)			\
+/* possibly ratelimited debug output */
+#define _batadv_dbg(type, bat_priv, ratelimited, fmt, arg...)			\
 	do {							\
-		if (atomic_read(&bat_priv->log_level) & type)	\
+		if (atomic_read(&bat_priv->log_level) & type && \
+		    (!ratelimited || net_ratelimit()))	\
 			batadv_debug_log(bat_priv, fmt, ## arg);\
 	}							\
 	while (0)
 #else /* !CONFIG_BATMAN_ADV_DEBUG */
-__printf(3, 4)
-static inline void batadv_dbg(int type __always_unused,
+__printf(4, 5)
+static inline void _batadv_dbg(int type __always_unused,
 			      struct batadv_priv *bat_priv __always_unused,
+			      int ratelimited __always_unused,
 			      const char *fmt __always_unused, ...)
 {
 }
 #endif
 
+#define batadv_dbg(type, bat_priv, arg...) \
+	_batadv_dbg(type, bat_priv, 0, ## arg)
+#define batadv_dbg_ratelimited(type, bat_priv, arg...) \
+	_batadv_dbg(type, bat_priv, 1, ## arg)
+
 #define batadv_info(net_dev, fmt, arg...)				\
 	do {								\
 		struct net_device *_netdev = (net_dev);                 \
diff --git a/routing.c b/routing.c
index 3514153..c679c6f 100644
--- a/routing.c
+++ b/routing.c
@@ -706,7 +706,7 @@  static int batadv_check_unicast_ttvn(struct batadv_priv *bat_priv,
 	if (batadv_tt_local_client_is_roaming(bat_priv, ethhdr->h_dest, vid)) {
 		if (batadv_reroute_unicast_packet(bat_priv, unicast_packet,
 						  ethhdr->h_dest, vid))
-			net_ratelimited_function(batadv_dbg, BATADV_DBG_TT,
+			batadv_dbg_ratelimited(BATADV_DBG_TT,
 						 bat_priv,
 						 "Rerouting unicast packet to %pM (dst=%pM): Local Roaming\n",
 						 unicast_packet->dest,
@@ -752,7 +752,7 @@  static int batadv_check_unicast_ttvn(struct batadv_priv *bat_priv,
 	 */
 	if (batadv_reroute_unicast_packet(bat_priv, unicast_packet,
 					  ethhdr->h_dest, vid)) {
-		net_ratelimited_function(batadv_dbg, BATADV_DBG_TT, bat_priv,
+		batadv_dbg_ratelimited(BATADV_DBG_TT, bat_priv,
 					 "Rerouting unicast packet to %pM (dst=%pM): TTVN mismatch old_ttvn=%u new_ttvn=%u\n",
 					 unicast_packet->dest, ethhdr->h_dest,
 					 old_ttvn, curr_ttvn);