Re: [Bridge] [PATCH] bridge: Fix format string for %ul

[Stephen Hemminger]

On Fri, 26 Aug 2016 23:10:28 -0400
Oleg Drokin  wrote:

> %ul would print an unsigned value and a letter l,
> likely it was %lu that was meant to print the long int,
> but in reality the values printed there are just regular signed
> ints, so just dropping the l altogether.
> 
> Signed-off-by: Oleg Drokin 
> ---
>  net/bridge/br_stp_bpdu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/bridge/br_stp_bpdu.c b/net/bridge/br_stp_bpdu.c
> index 5881fbc..15c4a9c 100644
> --- a/net/bridge/br_stp_bpdu.c
> +++ b/net/bridge/br_stp_bpdu.c
> @@ -230,7 +230,7 @@ void br_stp_rcv(const struct stp_proto *proto, struct 
> sk_buff *skb,
>   if (net_ratelimit())
>   br_notice(p->br,
> "port %u config from %pM"
> -   " (message_age %ul > max_age %ul)\n",
> +   " (message_age %u > max_age %u)\n",
> p->port_no,
> eth_hdr(skb)->h_source,
> bpdu.message_age, bpdu.max_age);

Could you make the format string a single line plwase.
And add Fixes tag.

Re: [Bridge] [PATCH] bridge: Fix format string for %ul

[Oleg Drokin]

On Aug 27, 2016, at 12:18 PM, Sergei Shtylyov wrote:

> Hello.
> 
> On 8/27/2016 6:58 PM, Oleg Drokin wrote:
> 
 %ul would print an unsigned value and a letter l,
 likely it was %lu that was meant to print the long int,
 but in reality the values printed there are just regular signed
>>> 
>>>  Signed? Then you need probably "%d" or "%i"…
>> 
>> They are signed in the struct definition, but in reality they
>> designate time, so could not be negative, I imagine?
> 
>   That doesn't matter. If the type is signed, it should be printed as signed.
> Doesn't gcc complain about the format specifiers not matching the values 
> passed?

only if the size differs.

I don't care either way.
I can change the struct to unsigned too, but if we want super correctness,
the problem lies deeper.

E.g. if we look how those times are derived, the age comes as:
static inline int br_get_ticks(const unsigned char *src)
{
unsigned long ticks = get_unaligned_be16(src);

return DIV_ROUND_UP(ticks * HZ, STP_HZ);
}

So we divide unsigned value by positive value and get a signed result ;)

there's a struct net_bridge_port with similar members in this are that are
unsigned long, so I guess we should convert to unsigned long in the
struct br_config_bpdu, do you want a patch for that separately or as part of 
this one?

> 
 ints, so just dropping the l altogether.
 
 Signed-off-by: Oleg Drokin 
>>> [...]
> 
> MBR, Sergei

Re: [Bridge] [PATCH] bridge: Fix format string for %ul

[Oleg Drokin]

On Aug 27, 2016, at 11:13 AM, Sergei Shtylyov wrote:

> Hello.
> 
> On 8/27/2016 6:10 AM, Oleg Drokin wrote:
> 
>> %ul would print an unsigned value and a letter l,
>> likely it was %lu that was meant to print the long int,
>> but in reality the values printed there are just regular signed
> 
>   Signed? Then you need probably "%d" or "%i"…

They are signed in the struct definition, but in reality they
designate time, so could not be negative, I imagine?

> 
>> ints, so just dropping the l altogether.
>> 
>> Signed-off-by: Oleg Drokin 
> [...]
> 
> MBR, Sergei

[Bridge] [PATCH] bridge: Fix format string for %ul

[Oleg Drokin]

%ul would print an unsigned value and a letter l,
likely it was %lu that was meant to print the long int,
but in reality the values printed there are just regular signed
ints, so just dropping the l altogether.

Signed-off-by: Oleg Drokin 
---
 net/bridge/br_stp_bpdu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/bridge/br_stp_bpdu.c b/net/bridge/br_stp_bpdu.c
index 5881fbc..15c4a9c 100644
--- a/net/bridge/br_stp_bpdu.c
+++ b/net/bridge/br_stp_bpdu.c
@@ -230,7 +230,7 @@ void br_stp_rcv(const struct stp_proto *proto, struct 
sk_buff *skb,
if (net_ratelimit())
br_notice(p->br,
  "port %u config from %pM"
- " (message_age %ul > max_age %ul)\n",
+ " (message_age %u > max_age %u)\n",
  p->port_no,
  eth_hdr(skb)->h_source,
  bpdu.message_age, bpdu.max_age);
-- 
2.7.4

Re: [Bridge] [PATCH] bridge: Fix format string for %ul

[Sergei Shtylyov]

Hello.

On 8/27/2016 6:58 PM, Oleg Drokin wrote:


%ul would print an unsigned value and a letter l,
likely it was %lu that was meant to print the long int,
but in reality the values printed there are just regular signed


  Signed? Then you need probably "%d" or "%i"…


They are signed in the struct definition, but in reality they
designate time, so could not be negative, I imagine?


   That doesn't matter. If the type is signed, it should be printed as signed.
Doesn't gcc complain about the format specifiers not matching the values passed?


ints, so just dropping the l altogether.

Signed-off-by: Oleg Drokin 

[...]


MBR, Sergei