From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Hemminger Subject: Re: [PATCH iproute2-next] iplink: add support for reporting multiple XDP programs Date: Fri, 13 Jul 2018 15:23:30 -0700 Message-ID: <20180713152330.7a2ec1dc@xeon-e3> References: <20180713204359.1161-1-jakub.kicinski@netronome.com> <20180713135941.3791c3ff@xeon-e3> <20180713142037.503b7b01@cakuba.lan> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: alexei.starovoitov@gmail.com, daniel@iogearbox.net, dsahern@gmail.com, netdev@vger.kernel.org, oss-drivers@netronome.com To: Jakub Kicinski Return-path: Received: from mail-pf0-f195.google.com ([209.85.192.195]:35785 "EHLO mail-pf0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725908AbeGMWkH (ORCPT ); Fri, 13 Jul 2018 18:40:07 -0400 Received: by mail-pf0-f195.google.com with SMTP id q7-v6so22259172pff.2 for ; Fri, 13 Jul 2018 15:23:32 -0700 (PDT) In-Reply-To: <20180713142037.503b7b01@cakuba.lan> Sender: netdev-owner@vger.kernel.org List-ID: On Fri, 13 Jul 2018 14:20:37 -0700 Jakub Kicinski wrote: > On Fri, 13 Jul 2018 13:59:41 -0700, Stephen Hemminger wrote: > > On Fri, 13 Jul 2018 13:43:59 -0700 > > Jakub Kicinski 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.