lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20180713152330.7a2ec1dc@xeon-e3>
Date:   Fri, 13 Jul 2018 15:23:30 -0700
From:   Stephen Hemminger <stephen@...workplumber.org>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     alexei.starovoitov@...il.com, daniel@...earbox.net,
        dsahern@...il.com, netdev@...r.kernel.org,
        oss-drivers@...ronome.com
Subject: Re: [PATCH iproute2-next] iplink: add support for reporting
 multiple XDP programs

On Fri, 13 Jul 2018 14:20:37 -0700
Jakub Kicinski <jakub.kicinski@...ronome.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@...ronome.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.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ