Re: [Cake] [PATCH 1/2] cake: print_uint format fixes
Kevin Darbyshire-Bryant writes:
>> On 12 Mar 2018, at 15:11, Stephen Hemminger
>> wrote:
>>
>> On Mon, 12 Mar 2018 10:56:09 +0100
>> Toke Høiland-Jørgensen 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
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake
Re: [Cake] [PATCH 1/2] cake: print_uint format fixes
> On 12 Mar 2018, at 15:11, Stephen Hemminger
> wrote:
>
> On Mon, 12 Mar 2018 10:56:09 +0100
> Toke Høiland-Jørgensen 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
--- Begin Message ---
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
---
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)
--- End Message ---
signature.asc
Description: Message signed with OpenPGP
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake
Re: [Cake] [PATCH 1/2] cake: print_uint format fixes
> On 12 Mar 2018, at 15:38, Toke Høiland-Jørgensen wrote:
>
> Stephen Hemminger 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
signature.asc
Description: Message signed with OpenPGP
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake
Re: [Cake] [PATCH 1/2] cake: print_uint format fixes
Stephen Hemminger 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
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake
Re: [Cake] [PATCH 1/2] cake: print_uint format fixes
On Mon, 12 Mar 2018 10:56:09 +0100
Toke Høiland-Jørgensen wrote:
> Kevin Darbyshire-Bryant writes:
>
> >> On 11 Mar 2018, at 23:34, Stephen Hemminger
> >> wrote:
> >>
> >> On Sun, 11 Mar 2018 22:10:51 +
> >> Kevin Darbyshire-Bryant 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)
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake
Re: [Cake] [PATCH 1/2] cake: print_uint format fixes
Kevin Darbyshire-Bryant writes:
>> On 11 Mar 2018, at 23:34, Stephen Hemminger
>> wrote:
>>
>> On Sun, 11 Mar 2018 22:10:51 +
>> Kevin Darbyshire-Bryant 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
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake
Re: [Cake] [PATCH 1/2] cake: print_uint format fixes
> On 11 Mar 2018, at 23:34, Stephen Hemminger > wrote: > > On Sun, 11 Mar 2018 22:10:51 + > Kevin Darbyshire-Bryant 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 signature.asc Description: Message signed with OpenPGP ___ Cake mailing list Cake@lists.bufferbloat.net https://lists.bufferbloat.net/listinfo/cake
Re: [Cake] [PATCH 1/2] cake: print_uint format fixes
On Sun, 11 Mar 2018 22:10:51 + Kevin Darbyshire-Bryant 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. ___ Cake mailing list Cake@lists.bufferbloat.net https://lists.bufferbloat.net/listinfo/cake
Re: [Cake] [PATCH 1/2] cake: print_uint format fixes
> On 11 Mar 2018, at 20:49, Toke Høiland-Jørgensen wrote: > > Thank you for the patch! :) > >> Signed-off-by: Kevin Darbyshire-Bryant >> --- >> 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 >> #include >> #include >> +#include >> >> #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 ___ Cake mailing list Cake@lists.bufferbloat.net https://lists.bufferbloat.net/listinfo/cake
Re: [Cake] [PATCH 1/2] cake: print_uint format fixes
Thank you for the patch! :) > Signed-off-by: Kevin Darbyshire-Bryant > --- > 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 > #include > #include > +#include > > #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 ___ Cake mailing list Cake@lists.bufferbloat.net https://lists.bufferbloat.net/listinfo/cake
[Cake] [PATCH 1/2] cake: print_uint format fixes
Signed-off-by: Kevin Darbyshire-Bryant
---
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
#include
#include
+#include
#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 mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake
