[Cake] fixing tc cake output since json-ification

[Cake] fixing tc cake output since json-ification

[Kevin Darbyshire-Bryant]

Here's 2 patches that fix (for me!) tc's strange output on a MIPS platform.

[PATCH 1/2] cake: print_uint format fixes
[PATCH 2/2] tc print_uint format fixes

[Cake] [PATCH 1/2] cake: print_uint format fixes

[Kevin Darbyshire-Bryant]

Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
---
 tc/q_cake.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/tc/q_cake.c b/tc/q_cake.c
index 44cadb63..f888bd2a 100644
--- a/tc/q_cake.c
+++ b/tc/q_cake.c
@@ -47,6 +47,7 @@
 #include <netinet/in.h>
 #include <arpa/inet.h>
 #include <string.h>
+#include <inttypes.h>
 
 #include "utils.h"
 #include "tc_util.h"
@@ -557,10 +558,10 @@ static int cake_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
 	else if (!raw)
 		print_string(PRINT_ANY, "atm", "%s ", "noatm");
 
-	print_uint(PRINT_ANY, "overhead", "overhead %d ", overhead);
+	print_uint(PRINT_ANY, "overhead", "overhead %" PRId64 " ", overhead);
 
 	if (mpu)
-		print_uint(PRINT_ANY, "mpu", "mpu %d ", mpu);
+		print_uint(PRINT_ANY, "mpu", "mpu %" PRIu64 " ", mpu);
 
 	if (memlimit) {
 		print_uint(PRINT_JSON, "memlimit", NULL, memlimit);
@@ -631,13 +632,13 @@ static int cake_print_xstats(struct qdisc_util *qu, FILE *f,
 	print_string(PRINT_FP, NULL, " capacity estimate: %s\n",
 		sprint_rate(stnc->capacity_estimate, b1));
 
-	print_uint(PRINT_ANY, "min_transport_size", " min/max transport layer size: %10u",
+	print_uint(PRINT_ANY, "min_transport_size", " min/max transport layer size: %10" PRIu64,
 		stnc->min_trnlen);
-	print_uint(PRINT_ANY, "max_transport_size", " /%8u\n", stnc->max_trnlen);
-	print_uint(PRINT_ANY, "min_adj_size", " min/max overhead-adjusted size: %8u",
+	print_uint(PRINT_ANY, "max_transport_size", " /%8" PRIu64 "\n", stnc->max_trnlen);
+	print_uint(PRINT_ANY, "min_adj_size", " min/max overhead-adjusted size: %8" PRIu64,
 		stnc->min_adjlen);
-	print_uint(PRINT_ANY, "max_adj_size", " /%8u\n", stnc->max_adjlen);
-	print_uint(PRINT_ANY, "avg_hdr_offset", " average transport hdr offset: %10u\n\n",
+	print_uint(PRINT_ANY, "max_adj_size", " /%8" PRIu64 "\n", stnc->max_adjlen);
+	print_uint(PRINT_ANY, "avg_hdr_offset", " average transport hdr offset: %10" PRIu64 "\n\n",
 		stnc->avg_trnoff);
 
 	if (is_json_context()) {
-- 
2.14.3 (Apple Git-98)

[Cake] [PATCH 2/2] tc print_uint format fixes

[Kevin Darbyshire-Bryant]

Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
---
 tc/tc_qdisc.c |  3 ++-
 tc/tc_util.c  | 24 ++++++++++++------------
 2 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/tc/tc_qdisc.c b/tc/tc_qdisc.c
index 70279b9d..3ec74a1c 100644
--- a/tc/tc_qdisc.c
+++ b/tc/tc_qdisc.c
@@ -20,6 +20,7 @@
 #include <string.h>
 #include <math.h>
 #include <malloc.h>
+#include <inttypes.h>
 
 #include "utils.h"
 #include "tc_util.h"
@@ -264,7 +265,7 @@ int print_qdisc(const struct sockaddr_nl *who,
 	}
 
 	if (t->tcm_info != 1)
-		print_uint(PRINT_ANY, "refcnt", "refcnt %u ", t->tcm_info);
+		print_uint(PRINT_ANY, "refcnt", "refcnt %" PRIu64 " ", t->tcm_info);
 
 	if (tb[TCA_HW_OFFLOAD] &&
 	    (rta_getattr_u8(tb[TCA_HW_OFFLOAD])))
diff --git a/tc/tc_util.c b/tc/tc_util.c
index aceb0d94..1b4501d8 100644
--- a/tc/tc_util.c
+++ b/tc/tc_util.c
@@ -714,10 +714,10 @@ void print_action_control(FILE *f, const char *prefix,
 	open_json_object("control_action");
 	print_string(PRINT_ANY, "type", "%s", action_n2a(action));
 	if (TC_ACT_EXT_CMP(action, TC_ACT_GOTO_CHAIN))
-		print_uint(PRINT_ANY, "chain", " chain %u",
+		print_uint(PRINT_ANY, "chain", " chain %" PRIu64 ,
 			   action & TC_ACT_EXT_VAL_MASK);
 	if (TC_ACT_EXT_CMP(action, TC_ACT_JUMP))
-		print_uint(PRINT_ANY, "jump", " %u",
+		print_uint(PRINT_ANY, "jump", " %" PRIu64 ,
 			   action & TC_ACT_EXT_VAL_MASK);
 	close_json_object();
 	print_string(PRINT_FP, NULL, "%s", suffix);
@@ -770,17 +770,17 @@ void print_tm(FILE *f, const struct tcf_t *tm)
 
 	if (tm->install != 0) {
 		print_uint(PRINT_JSON, "installed", NULL, tm->install);
-		print_uint(PRINT_FP, NULL, " installed %u sec",
+		print_uint(PRINT_FP, NULL, " installed %" PRIu64 " sec",
 			   (unsigned int)(tm->install/hz));
 	}
 	if (tm->lastuse != 0) {
 		print_uint(PRINT_JSON, "last_used", NULL, tm->lastuse);
-		print_uint(PRINT_FP, NULL, " used %u sec",
+		print_uint(PRINT_FP, NULL, " used %" PRIu64 " sec",
 			   (unsigned int)(tm->lastuse/hz));
 	}
 	if (tm->expires != 0) {
 		print_uint(PRINT_JSON, "expires", NULL, tm->expires);
-		print_uint(PRINT_FP, NULL, " expires %u sec",
+		print_uint(PRINT_FP, NULL, " expires %" PRIu64 " sec",
 			   (unsigned int)(tm->expires/hz));
 	}
 }
@@ -798,17 +798,17 @@ void print_tcstats2_attr(FILE *fp, struct rtattr *rta, char *prefix, struct rtat
 		memcpy(&bs, RTA_DATA(tbs[TCA_STATS_BASIC]), MIN(RTA_PAYLOAD(tbs[TCA_STATS_BASIC]), sizeof(bs)));
 		print_string(PRINT_FP, NULL, "%s", prefix);
 		print_lluint(PRINT_ANY, "bytes", "Sent %llu bytes", bs.bytes);
-		print_uint(PRINT_ANY, "packets", " %u pkt", bs.packets);
+		print_uint(PRINT_ANY, "packets", " %" PRIu64 " pkt", bs.packets);
 	}
 
 	if (tbs[TCA_STATS_QUEUE]) {
 		struct gnet_stats_queue q = {0};
 
 		memcpy(&q, RTA_DATA(tbs[TCA_STATS_QUEUE]), MIN(RTA_PAYLOAD(tbs[TCA_STATS_QUEUE]), sizeof(q)));
-		print_uint(PRINT_ANY, "drops", " (dropped %u", q.drops);
-		print_uint(PRINT_ANY, "overlimits", ", overlimits %u",
+		print_uint(PRINT_ANY, "drops", " (dropped %" PRIu64, q.drops);
+		print_uint(PRINT_ANY, "overlimits", ", overlimits %" PRIu64,
 			   q.overlimits);
-		print_uint(PRINT_ANY, "requeues", " requeues %u) ", q.requeues);
+		print_uint(PRINT_ANY, "requeues", " requeues %" PRIu64 ") ", q.requeues);
 	}
 
 	if (tbs[TCA_STATS_RATE_EST64]) {
@@ -833,7 +833,7 @@ void print_tcstats2_attr(FILE *fp, struct rtattr *rta, char *prefix, struct rtat
 		print_uint(PRINT_JSON, "rate", NULL, re.bps);
 		print_string(PRINT_FP, NULL, "rate %s",
 			     sprint_rate(re.bps, b1));
-		print_uint(PRINT_ANY, "pps", " %upps", re.pps);
+		print_uint(PRINT_ANY, "pps", " %" PRIu64 "pps", re.pps);
 	}
 
 	if (tbs[TCA_STATS_QUEUE]) {
@@ -845,8 +845,8 @@ void print_tcstats2_attr(FILE *fp, struct rtattr *rta, char *prefix, struct rtat
 		print_uint(PRINT_JSON, "backlog", NULL, q.backlog);
 		print_string(PRINT_FP, NULL, "backlog %s",
 			     sprint_size(q.backlog, b1));
-		print_uint(PRINT_ANY, "qlen", " %up", q.qlen);
-		print_uint(PRINT_FP, NULL, " requeues %u", q.requeues);
+		print_uint(PRINT_ANY, "qlen", " %" PRIu64 "p", q.qlen);
+		print_uint(PRINT_FP, NULL, " requeues %" PRIu64, q.requeues);
 	}
 
 	if (xstats)
-- 
2.14.3 (Apple Git-98)

Re: [Cake] [PATCH 1/2] cake: print_uint format fixes

[Toke Høiland-Jørgensen]

Thank you for the patch! :)

> Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
> ---
>  tc/q_cake.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/tc/q_cake.c b/tc/q_cake.c
> index 44cadb63..f888bd2a 100644
> --- a/tc/q_cake.c
> +++ b/tc/q_cake.c
> @@ -47,6 +47,7 @@
>  #include <netinet/in.h>
>  #include <arpa/inet.h>
>  #include <string.h>
> +#include <inttypes.h>
>  
>  #include "utils.h"
>  #include "tc_util.h"
> @@ -557,10 +558,10 @@ static int cake_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
>  	else if (!raw)
>  		print_string(PRINT_ANY, "atm", "%s ", "noatm");
>  
> -	print_uint(PRINT_ANY, "overhead", "overhead %d ", overhead);
> +	print_uint(PRINT_ANY, "overhead", "overhead %" PRId64 " ",
> overhead);

Guess this should actually be print_int(), since overhead can be
negative?

-Toke

Re: [Cake] [PATCH 2/2] tc print_uint format fixes

[Toke Høiland-Jørgensen]

Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk> writes:

> Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
> ---
>  tc/tc_qdisc.c |  3 ++-
>  tc/tc_util.c  | 24 ++++++++++++------------
>  2 files changed, 14 insertions(+), 13 deletions(-)

Care to submit this to upstream iproute2? Amending the commit message
with an explanation of why the change is needed and sending the same
patch to netdev@vger.kernel.org should be enough, I think :)

-Toke

Re: [Cake] [PATCH 2/2] tc print_uint format fixes

[Kevin Darbyshire-Bryant]

> On 11 Mar 2018, at 20:50, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> 
> Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk> writes:
> 
>> Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
>> ---
>> tc/tc_qdisc.c |  3 ++-
>> tc/tc_util.c  | 24 ++++++++++++------------
>> 2 files changed, 14 insertions(+), 13 deletions(-)
> 
> Care to submit this to upstream iproute2? Amending the commit message
> with an explanation of why the change is needed and sending the same
> patch to netdev@vger.kernel.org should be enough, I think :)

I wanted a sanity check first :-)  And there’s more to do.  A LOT more!

Is it sane? 


> 
> -Toke
> _______________________________________________
> Cake mailing list
> Cake@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/cake


Cheers,

Kevin D-B

012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A

Re: [Cake] [PATCH 2/2] tc print_uint format fixes

[Toke Høiland-Jørgensen]

Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:

>> On 11 Mar 2018, at 20:50, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>> 
>> Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk> writes:
>> 
>>> Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
>>> ---
>>> tc/tc_qdisc.c |  3 ++-
>>> tc/tc_util.c  | 24 ++++++++++++------------
>>> 2 files changed, 14 insertions(+), 13 deletions(-)
>> 
>> Care to submit this to upstream iproute2? Amending the commit message
>> with an explanation of why the change is needed and sending the same
>> patch to netdev@vger.kernel.org should be enough, I think :)
>
> I wanted a sanity check first :-)  And there’s more to do.  A LOT more!
>
> Is it sane?

Apart from the fact that it is really grating that a function called
"print_uint()" needs this weird 64-bit format string include. But that
is probably an issue that upstream should deal with.

Also, the inttypes.h include is missing from tc_util.c in your patch...

Other than that, compiles and works for me on x86_64...

-Toke

Re: [Cake] [PATCH 1/2] cake: print_uint format fixes

[Kevin Darbyshire-Bryant]

> On 11 Mar 2018, at 20:49, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> 
> Thank you for the patch! :)
> 
>> Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
>> ---
>> tc/q_cake.c | 15 ++++++++-------
>> 1 file changed, 8 insertions(+), 7 deletions(-)
>> 
>> diff --git a/tc/q_cake.c b/tc/q_cake.c
>> index 44cadb63..f888bd2a 100644
>> --- a/tc/q_cake.c
>> +++ b/tc/q_cake.c
>> @@ -47,6 +47,7 @@
>> #include <netinet/in.h>
>> #include <arpa/inet.h>
>> #include <string.h>
>> +#include <inttypes.h>
>> 
>> #include "utils.h"
>> #include "tc_util.h"
>> @@ -557,10 +558,10 @@ static int cake_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
>> 	else if (!raw)
>> 		print_string(PRINT_ANY, "atm", "%s ", "noatm");
>> 
>> -	print_uint(PRINT_ANY, "overhead", "overhead %d ", overhead);
>> +	print_uint(PRINT_ANY, "overhead", "overhead %" PRId64 " ",
>> overhead);
> 
> Guess this should actually be print_int(), since overhead can be
> negative?

Tried it, and you’d sort of guess wrong.  I’ll write it up tomorrow ‘properly’ but ‘int’ is int length whereas uint is uint64 length.  On big endian it goes wrong.

Anyway, glad you’ve tested on something little endian.  I’ll try to submit a patch upstream as requested…very busy over next 3 days doing $dayjob so may take a little while.  Thanks for boosting confidence that I’ve not broken it on architectures it used to work on :-)

> 
> -Toke


Cheers,

Kevin D-B

012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A

Re: [Cake] [PATCH 1/2] cake: print_uint format fixes

[Stephen Hemminger]

On Sun, 11 Mar 2018 22:10:51 +0000
Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> wrote:

>  negative?  
> 
> Tried it, and you’d sort of guess wrong.  I’ll write it up tomorrow ‘properly’ but ‘int’ is int length whereas uint is uint64 length.  On big endian it goes wrong.
> 
> Anyway, glad you’ve tested on something little endian.  I’ll try to submit a patch upstream as requested…very busy over next 3 days doing $dayjob so may take a little while.  Thanks for boosting confidence that I’ve not broken it on architectures it used to work on :-)

print_uint should be unsigned only.

When printing json your version won't work with negative values.

Re: [Cake] [PATCH 1/2] cake: print_uint format fixes

[Kevin Darbyshire-Bryant]

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



> On 11 Mar 2018, at 23:34, Stephen Hemminger <stephen@networkplumber.org> wrote:
> 
> On Sun, 11 Mar 2018 22:10:51 +0000
> Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> wrote:
> 
>> negative?
>> 
>> Tried it, and you’d sort of guess wrong.  I’ll write it up tomorrow ‘properly’ but ‘int’ is int length whereas uint is uint64 length.  On big endian it goes wrong.
>> 
>> Anyway, glad you’ve tested on something little endian.  I’ll try to submit a patch upstream as requested…very busy over next 3 days doing $dayjob so may take a little while.  Thanks for boosting confidence that I’ve not broken it on architectures it used to work on :-)
> 
> print_uint should be unsigned only.
> 
> When printing json your version won't work with negative values.

Yes, it should be:  print_int(PRINT_ANY, "overhead", "overhead %d ", overhead);   - certainly that works on my 32 bit big endian box.

Using the ‘PRId64’ macro won’t work because print_int is using ‘int’ type internally whereas print_uint uses ‘uint64_t’ internally.  So the format string has to have knowledge of the internal format, *but* there’s no clue of the difference in internal format offered by the function name i.e.  print_int vs print_uint.

I’d argue it makes more sense to have: print_int/print_uint as the native int length, that hopefully match up with %u & %d  and then have print_int64/print_uint64 where use of formats PRId64 & PRIu64 is advised.

But I’m definitely arguing from a position of lack of knowledge/experience.


Cheers,

Kevin D-B

012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Cake] [PATCH 1/2] cake: print_uint format fixes

[Toke Høiland-Jørgensen]

Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:

>> On 11 Mar 2018, at 23:34, Stephen Hemminger <stephen@networkplumber.org> wrote:
>> 
>> On Sun, 11 Mar 2018 22:10:51 +0000
>> Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> wrote:
>> 
>>> negative?
>>> 
>>> Tried it, and you’d sort of guess wrong. I’ll write it up tomorrow
>>> ‘properly’ but ‘int’ is int length whereas uint is uint64 length. On
>>> big endian it goes wrong.
>>> 
>>> Anyway, glad you’ve tested on something little endian. I’ll try to
>>> submit a patch upstream as requested…very busy over next 3 days
>>> doing $dayjob so may take a little while. Thanks for boosting
>>> confidence that I’ve not broken it on architectures it used to work
>>> on :-)
>> 
>> print_uint should be unsigned only.
>> 
>> When printing json your version won't work with negative values.
>
> Yes, it should be: print_int(PRINT_ANY, "overhead", "overhead %d ",
> overhead); - certainly that works on my 32 bit big endian box.

Yeah, this is fixed in the tc-adv git repo now :)

> Using the ‘PRId64’ macro won’t work because print_int is using ‘int’
> type internally whereas print_uint uses ‘uint64_t’ internally. So the
> format string has to have knowledge of the internal format, *but*
> there’s no clue of the difference in internal format offered by the
> function name i.e. print_int vs print_uint.
>
> I’d argue it makes more sense to have: print_int/print_uint as the
> native int length, that hopefully match up with %u & %d and then have
> print_int64/print_uint64 where use of formats PRId64 & PRIu64 is
> advised.

Yes, this was basically what I meant by "grating"; I really do agree
that this API is confusing.

Stephen, would you accept patches to fix the API (to add
print_{u,}int64() variants and turn print_uint() into native-int size)?
Or should we stick with the API currently there and live with the
inconsistency? :)

-Toke

Re: [Cake] [PATCH 1/2] cake: print_uint format fixes

[Stephen Hemminger]

On Mon, 12 Mar 2018 10:56:09 +0100
Toke Høiland-Jørgensen <toke@toke.dk> wrote:

> Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:
> 
> >> On 11 Mar 2018, at 23:34, Stephen Hemminger <stephen@networkplumber.org> wrote:
> >> 
> >> On Sun, 11 Mar 2018 22:10:51 +0000
> >> Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> wrote:
> >>   
> >>> negative?
> >>> 
> >>> Tried it, and you’d sort of guess wrong. I’ll write it up tomorrow
> >>> ‘properly’ but ‘int’ is int length whereas uint is uint64 length. On
> >>> big endian it goes wrong.
> >>> 
> >>> Anyway, glad you’ve tested on something little endian. I’ll try to
> >>> submit a patch upstream as requested…very busy over next 3 days
> >>> doing $dayjob so may take a little while. Thanks for boosting
> >>> confidence that I’ve not broken it on architectures it used to work
> >>> on :-)  
> >> 
> >> print_uint should be unsigned only.
> >> 
> >> When printing json your version won't work with negative values.  
> >
> > Yes, it should be: print_int(PRINT_ANY, "overhead", "overhead %d ",
> > overhead); - certainly that works on my 32 bit big endian box.  
> 
> Yeah, this is fixed in the tc-adv git repo now :)
> 
> > Using the ‘PRId64’ macro won’t work because print_int is using ‘int’
> > type internally whereas print_uint uses ‘uint64_t’ internally. So the
> > format string has to have knowledge of the internal format, *but*
> > there’s no clue of the difference in internal format offered by the
> > function name i.e. print_int vs print_uint.
> >
> > I’d argue it makes more sense to have: print_int/print_uint as the
> > native int length, that hopefully match up with %u & %d and then have
> > print_int64/print_uint64 where use of formats PRId64 & PRIu64 is
> > advised.  
> 
> Yes, this was basically what I meant by "grating"; I really do agree
> that this API is confusing.
> 
> Stephen, would you accept patches to fix the API (to add
> print_{u,}int64() variants and turn print_uint() into native-int size)?
> Or should we stick with the API currently there and live with the
> inconsistency? :)
> 
> -Toke

I agree print_int should take int, print_uint should take unsigned int, and there should
be print_u64 (and print_u32, print_u8)

Re: [Cake] [PATCH 1/2] cake: print_uint format fixes

[Toke Høiland-Jørgensen]

Stephen Hemminger <stephen@networkplumber.org> writes:

>> > Using the ‘PRId64’ macro won’t work because print_int is using ‘int’
>> > type internally whereas print_uint uses ‘uint64_t’ internally. So the
>> > format string has to have knowledge of the internal format, *but*
>> > there’s no clue of the difference in internal format offered by the
>> > function name i.e. print_int vs print_uint.
>> >
>> > I’d argue it makes more sense to have: print_int/print_uint as the
>> > native int length, that hopefully match up with %u & %d and then have
>> > print_int64/print_uint64 where use of formats PRId64 & PRIu64 is
>> > advised.  
>> 
>> Yes, this was basically what I meant by "grating"; I really do agree
>> that this API is confusing.
>> 
>> Stephen, would you accept patches to fix the API (to add
>> print_{u,}int64() variants and turn print_uint() into native-int size)?
>> Or should we stick with the API currently there and live with the
>> inconsistency? :)
>> 
>> -Toke
>
> I agree print_int should take int, print_uint should take unsigned
> int, and there should be print_u64 (and print_u32, print_u8)

Cool. Kevin, do you feel like submitting a patch? :)

-Toke

Re: [Cake] [PATCH 1/2] cake: print_uint format fixes

[Kevin Darbyshire-Bryant]

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



> On 12 Mar 2018, at 15:38, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> 
> Stephen Hemminger <stephen@networkplumber.org> writes:
> 
>>>> Using the ‘PRId64’ macro won’t work because print_int is using ‘int’
>>>> type internally whereas print_uint uses ‘uint64_t’ internally. So the
>>>> format string has to have knowledge of the internal format, *but*
>>>> there’s no clue of the difference in internal format offered by the
>>>> function name i.e. print_int vs print_uint.
>>>> 
>>>> I’d argue it makes more sense to have: print_int/print_uint as the
>>>> native int length, that hopefully match up with %u & %d and then have
>>>> print_int64/print_uint64 where use of formats PRId64 & PRIu64 is
>>>> advised.
>>> 
>>> Yes, this was basically what I meant by "grating"; I really do agree
>>> that this API is confusing.
>>> 
>>> Stephen, would you accept patches to fix the API (to add
>>> print_{u,}int64() variants and turn print_uint() into native-int size)?
>>> Or should we stick with the API currently there and live with the
>>> inconsistency? :)
>>> 
>>> -Toke
>> 
>> I agree print_int should take int, print_uint should take unsigned
>> int, and there should be print_u64 (and print_u32, print_u8)
> 
> Cool. Kevin, do you feel like submitting a patch? :)
> 
> -Toke

I’m in full on $paidwork broadcast sound supervisor mode for the next 4 days, so it won’t be done in a screaming rush.

Talk about patch feature creep ;-)

Cheers,

Kevin D-B

012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Cake] [PATCH 1/2] cake: print_uint format fixes

[Kevin Darbyshire-Bryant]

[-- Attachment #1.1: Type: text/plain, Size: 1062 bytes --]



> On 12 Mar 2018, at 15:11, Stephen Hemminger <stephen@networkplumber.org> wrote:
> 
> On Mon, 12 Mar 2018 10:56:09 +0100
> Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>> 
>> Stephen, would you accept patches to fix the API (to add
>> print_{u,}int64() variants and turn print_uint() into native-int size)?
>> Or should we stick with the API currently there and live with the
>> inconsistency? :)
>> 
>> -Toke
> 
> I agree print_int should take int, print_uint should take unsigned int, and there should
> be print_u64 (and print_u32, print_u8)

Submitted a patch to netdev but almost certainly did it wrong/based on wrong tree etc ‘cos I don’t normally inhabit that space. The quite simple attached patch restores corrrect functioning on my BE MIPS box.  Validation that the MANY calls to print_uint are using the correct type is left as an exercise for the reader :-)  But at least there’s no longer a hidden promotion from uint to uint_64t going on.


Cheers,

Kevin D-B

012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A

[-- Attachment #1.2: [PATCH] json_print: fix print_uint with helper type extensions.eml --]
[-- Type: message/rfc822, Size: 8546 bytes --]

From: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
To: netdev@vger.kernel.org
Cc: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
Subject: [PATCH] json_print: fix print_uint with helper type extensions
Date: Thu, 15 Mar 2018 20:07:20 +0000
Message-ID: <20180315200720.829-1-ldir@darbyshire-bryant.me.uk>

Introduce print helper functions for int, uint, explicit int32, uint32,
int64 & uint64.

print_int used 'int' type internally, whereas print_uint used 'uint64_t'

These helper functions eventually call vfprintf(fp, fmt, args) which is
a variable argument list function and is dependent upon 'fmt' containing
correct information about the length of the passed arguments.

Unfortunately print_int v print_uint offered no clue to the programmer
that internally passed ints to print_uint were being promoted to 64bits,
thus the format passed in 'fmt' string vs the actual passed integer
could be different lengths.  This is even more interesting on big endian
architectures where 'vfprintf' would be looking in the middle of an
int64 type and hence produced wildly incorrect values in tc qdisc
output.

print_u/int now stick with native int size.  print_u/int32 & print
u/int64 functions offer explicit integer sizes.

To portably use these formats you should use the relevant PRIdN or PRIuN
formats as defined in inttypes.h

e.g.

print_uint64(PRINT_ANY, "refcnt", "refcnt %" PRIu64 " ", t->tcm_info)

Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
---
 include/json_print.h | 6 +++++-
 lib/json_print.c     | 6 +++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/include/json_print.h b/include/json_print.h
index 2ca7830a..fb62b142 100644
--- a/include/json_print.h
+++ b/include/json_print.h
@@ -56,10 +56,14 @@ void close_json_array(enum output_type type, const char *delim);
 		print_color_##type_name(t, COLOR_NONE, key, fmt, value);	\
 	}
 _PRINT_FUNC(int, int);
+_PRINT_FUNC(uint, unsigned int);
 _PRINT_FUNC(bool, bool);
 _PRINT_FUNC(null, const char*);
 _PRINT_FUNC(string, const char*);
-_PRINT_FUNC(uint, uint64_t);
+_PRINT_FUNC(int32, int32_t);
+_PRINT_FUNC(uint32, uint32_t);
+_PRINT_FUNC(int64, int64_t);
+_PRINT_FUNC(uint64, uint64_t);
 _PRINT_FUNC(hu, unsigned short);
 _PRINT_FUNC(hex, unsigned int);
 _PRINT_FUNC(0xhex, unsigned int);
diff --git a/lib/json_print.c b/lib/json_print.c
index 6518ba98..12ee26df 100644
--- a/lib/json_print.c
+++ b/lib/json_print.c
@@ -117,8 +117,12 @@ void close_json_array(enum output_type type, const char *str)
 		}							\
 	}
 _PRINT_FUNC(int, int);
+_PRINT_FUNC(uint, unsigned int);
 _PRINT_FUNC(hu, unsigned short);
-_PRINT_FUNC(uint, uint64_t);
+_PRINT_FUNC(int32, int32_t);
+_PRINT_FUNC(uint32, uint32_t);
+_PRINT_FUNC(int64, int64_t);
+_PRINT_FUNC(uint64, uint64_t);
 _PRINT_FUNC(lluint, unsigned long long int);
 _PRINT_FUNC(float, double);
 #undef _PRINT_FUNC
-- 
2.14.3 (Apple Git-98)


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Cake] [PATCH 1/2] cake: print_uint format fixes

[Toke Høiland-Jørgensen]

Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:

>> On 12 Mar 2018, at 15:11, Stephen Hemminger <stephen@networkplumber.org> wrote:
>> 
>> On Mon, 12 Mar 2018 10:56:09 +0100
>> Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>>> 
>>> Stephen, would you accept patches to fix the API (to add
>>> print_{u,}int64() variants and turn print_uint() into native-int size)?
>>> Or should we stick with the API currently there and live with the
>>> inconsistency? :)
>>> 
>>> -Toke
>> 
>> I agree print_int should take int, print_uint should take unsigned int, and there should
>> be print_u64 (and print_u32, print_u8)
>
> Submitted a patch to netdev but almost certainly did it wrong/based on
> wrong tree etc ‘cos I don’t normally inhabit that space. The quite
> simple attached patch restores corrrect functioning on my BE MIPS box.
> Validation that the MANY calls to print_uint are using the correct
> type is left as an exercise for the reader :-) But at least there’s no
> longer a hidden promotion from uint to uint_64t going on.

Awesome! Good work :)

-Toke