Re: [PATCH iproute2-next] iplink: add support for reporting multiple XDP programs

[Stephen Hemminger <stephen@networkplumber.org>]

On Fri, 13 Jul 2018 14:20:37 -0700
Jakub Kicinski <jakub.kicinski@netronome.com> wrote:

> On Fri, 13 Jul 2018 13:59:41 -0700, Stephen Hemminger wrote:
> > On Fri, 13 Jul 2018 13:43:59 -0700
> > Jakub Kicinski <jakub.kicinski@netronome.com> wrote:
> >   
> > >  
> > > +static void xdp_dump_prog_one(FILE *fp, struct rtattr *tb[IFLA_XDP_MAX + 1],
> > > +			      __u32 attr, bool link, bool details, char *pfx)
> > > +{
> > > +	__u32 prog_id;
> > > +
> > > +	if (!tb[attr])
> > > +		return;
> > > +
> > > +	prog_id = rta_getattr_u32(tb[attr]);
> > > +	if (!details) {
> > > +		if (prog_id && !link && attr == IFLA_XDP_PROG_ID)
> > > +			fprintf(fp, "/id:%u", prog_id);
> > > +		return;
> > > +	}
> > > +
> > > +	if (prog_id) {
> > > +		fprintf(fp, "%s    prog/xdp%s ", _SL_, pfx);
> > > +		bpf_dump_prog_info(fp, prog_id);
> > > +	}    
> > 
> > Maybe const char *pfx.  
> 
> Will do!  Looking again at this code, I think I will also do this:
> 
> diff --git a/ip/iplink_xdp.c b/ip/iplink_xdp.c
> index 0328bc01a981..68629834cc00 100644
> --- a/ip/iplink_xdp.c
> +++ b/ip/iplink_xdp.c
> @@ -121,6 +121,12 @@ static void xdp_dump_json(struct rtattr *tb[IFLA_XDP_MAX + 1])
>         xdp_dump_json_one(tb, IFLA_XDP_SKB_PROG_ID, XDP_ATTACHED_SKB);
>         xdp_dump_json_one(tb, IFLA_XDP_DRV_PROG_ID, XDP_ATTACHED_DRV);
>         xdp_dump_json_one(tb, IFLA_XDP_HW_PROG_ID, XDP_ATTACHED_HW);
> +       /* Older kernel - use IFLA_XDP_PROG_ID */
> +       if (tb[IFLA_XDP_PROG_ID] &&
> +           !(tb[IFLA_XDP_ATTACHED_SKB] ||
> +             tb[IFLA_XDP_ATTACHED_DRV] ||
> +             tb[IFLA_XDP_ATTACHED_HW]))
> +               xdp_dump_json_one(tb, IFLA_XDP_PROG_ID, mode);
>         close_json_array(PRINT_JSON, NULL);
>  
>         close_json_object();
> 
> So that on older kernels we will still be able to depend on the
> contents of the "attached" array, even if kernel does not know to
> report program per-mode, yet.
> 
> > I prefer to not use "printf(fp," and use print_string(PRINT_FP, NULL, "%s", ...)
> > because otherwise you end up mixing strings and json format output in the
> > same result.
> > 
> > You should be able to do
> > 	tc -j ... 
> > and always get valid JSON output.
> > 
> > One quick way to test json validation is to pipe it into python:
> > 	tc -j ... | python -mjson.tool  
> 
> Note that XDP has separate print functions for plain text and JSON, and
> the flow gets separated early on:
> 
> 	mode = rta_getattr_u8(tb[IFLA_XDP_ATTACHED]);
> 	if (mode == XDP_ATTACHED_NONE)
> 		return;
> 	else if (is_json_context())
> 		return details ? (void)0 : xdp_dump_json(tb);
> 
> 	... non-JSON handling follows...
> 
> The use of fprintfs is therefore okay.  Do you have a preference for
> using the wrapper, even if fprintf is safe?  It's brevity vs
> consistency, I guess.  We'd need a separate patch for that, 'cause I'm
> not touching all the fprintfs in the file, anyway.

The only preference for the wrapper is that it is easy way to make
sure all code is JSON aware.  Since fp is always stdout in current
code, maybe just convert to printf.