Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Kevin Darbyshire-Bryant]

> On 8 Mar 2018, at 11:21, Toke Høiland-Jørgensen  wrote:
> 
>> 
>> Oh and curiously the bad values go away if you ask for json output
>> it’s much better.  Which rather points at a ‘feature’ of the
>> ‘print_string’ behaviour.
> 
> Right. Well, the print_* functions are behind several levels of
> pre-processor indirection, so not quite obvious what's going on here.
> Don't really see why they should spit out garbage values, though.
> 
> 
> Stephen, do you have any ideas?
> 
> -Toke

Right, cracked it and it’s horrible!

print_uint is expanded thus:  Note the type of value uint64_t

  void print_color_uint(enum output_type t, enum color_attr color, 
const char *key, const char *fmt, uint64_t value);
static inline void print_uint  (enum output_type t,
const char *key, const char *fmt, uint64_t value)
 { print_color_uint( t, COLOR_NONE, 
   key, fmt,  value); };

So far so good.

print_color_uint expands to:

 void print_color_uint(enum output_type t, enum color_attr color, 
const char *key, const char *fmt, uint64_t value)
 {
  if (((t & PRINT_JSON || t & PRINT_ANY) && _jw))
{ if (!key) jsonw_uint(_jw, value);
  else  jsonw_uint_field(_jw, key, value);
}
  else if ((!_jw && (t & PRINT_FP || t & PRINT_ANY)))
{ color_fprintf( (stdout) , color, fmt, value);
}
 };

Again, no issue and we eventually call color_fprintf

int color_fprintf(FILE *fp, enum color_attr attr, const char *fmt, ...)
{
int ret = 0;
va_list args;

va_start(args, fmt);

if (!color_is_enabled || attr == COLOR_NONE) {
ret = vfprintf(fp, fmt, args);
goto end;
}


Now, color_printf is a variable argument list function and as such is dependent 
upon being told the correct size of argument variables in the fmt variable.  
And that’s our problem, we’re passing a 64bit integer but telling the format 
string that it’s ‘int’…which I’m guessing can be variable sizes depending on 
architecture, as can the endianness.

If we instead do (in q_cake.c)

#include 

print_uint(PRINT_ANY, "min_transport_size", " min/max transport layer size: 
%10" PRIu64, stnc->min_trnlen);

it works.  This needs sanity checking by a clever person.

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] q_cake: Update xstats format to use per-tin structure

[Luis E. Garcia]

Nice work Kevin.

On Sat, Mar 10, 2018 at 09:56 Kevin Darbyshire-Bryant <
ke...@darbyshire-bryant.me.uk> wrote:

>
>
> > On 8 Mar 2018, at 11:21, Toke Høiland-Jørgensen  wrote:
> >
> >>
> >> Oh and curiously the bad values go away if you ask for json output
> >> it’s much better.  Which rather points at a ‘feature’ of the
> >> ‘print_string’ behaviour.
> >
> > Right. Well, the print_* functions are behind several levels of
> > pre-processor indirection, so not quite obvious what's going on here.
> > Don't really see why they should spit out garbage values, though.
> >
> >
> > Stephen, do you have any ideas?
> >
> > -Toke
>
> Right, cracked it and it’s horrible!
>
> print_uint is expanded thus:  Note the type of value uint64_t
>
>   void print_color_uint(enum output_type t, enum color_attr
> color, const char *key, const char *fmt, uint64_t value);
> static inline void print_uint  (enum output_type t,
> const char *key, const char *fmt, uint64_t value)
>  { print_color_uint( t, COLOR_NONE,
> key, fmt,  value); };
>
> So far so good.
>
> print_color_uint expands to:
>
>  void print_color_uint(enum output_type t, enum color_attr
> color, const char *key, const char *fmt, uint64_t value)
>  {
>   if (((t & PRINT_JSON || t & PRINT_ANY) && _jw))
> { if (!key) jsonw_uint(_jw, value);
>   else  jsonw_uint_field(_jw, key, value);
> }
>   else if ((!_jw && (t & PRINT_FP || t & PRINT_ANY)))
> { color_fprintf( (stdout) , color, fmt, value);
> }
>  };
>
> Again, no issue and we eventually call color_fprintf
>
> int color_fprintf(FILE *fp, enum color_attr attr, const char *fmt, ...)
> {
> int ret = 0;
> va_list args;
>
> va_start(args, fmt);
>
> if (!color_is_enabled || attr == COLOR_NONE) {
> ret = vfprintf(fp, fmt, args);
> goto end;
> }
>
>
> Now, color_printf is a variable argument list function and as such is
> dependent upon being told the correct size of argument variables in the fmt
> variable.  And that’s our problem, we’re passing a 64bit integer but
> telling the format string that it’s ‘int’…which I’m guessing can be
> variable sizes depending on architecture, as can the endianness.
>
> If we instead do (in q_cake.c)
>
> #include 
>
> print_uint(PRINT_ANY, "min_transport_size", " min/max transport layer
> size: %10" PRIu64, stnc->min_trnlen);
>
> it works.  This needs sanity checking by a clever person.
>
> 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
>
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake