Date:   Wed, 22 May 2019 18:23:28 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Andrii Nakryiko <andriin@...com>, Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Networking <netdev@...r.kernel.org>, bpf@...r.kernel.org,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH bpf-next 10/12] bpftool: add C output format option to
 btf dump subcommand

On Wed, 22 May 2019 17:58:23 -0700, Andrii Nakryiko wrote:
> On Wed, May 22, 2019 at 5:25 PM Jakub Kicinski wrote:
> > On Wed, 22 May 2019 12:50:51 -0700, Andrii Nakryiko wrote:  
> > > + * Copyright (C) 2019 Facebook
> > > + */
> > >
> > >  #include <errno.h>
> > >  #include <fcntl.h>
> > > @@ -340,11 +347,48 @@ static int dump_btf_raw(const struct btf *btf,
> > >       return 0;
> > >  }
> > >
> > > +static void btf_dump_printf(void *ctx, const char *fmt, va_list args)
> > > +{
> > > +     vfprintf(stdout, fmt, args);
> > > +}
> > > +
> > > +static int dump_btf_c(const struct btf *btf,
> > > +                   __u32 *root_type_ids, int root_type_cnt)  
> >
> > Please break the line after static int.  
> 
> I don't mind, but it seems that prevalent formatting for such cases
> (at least in bpftool code base) is aligning arguments and not break
> static <return type> into separate line:
> 
> // multi-line function definitions with static on the same line
> $ rg '^static \w+.*\([^\)]*$' | wc -l
> 45
> // multi-line function definitions with static on separate line
> $ rg '^static \w+[^\(\{;]*$' | wc -l
> 12
> 
> So I don't mind changing, but which one is canonical way of formatting?

Not really, just my preference :)

In my experience having the return type on a separate line if its
longer than a few chars is the simplest rule for consistent and good
looking code.

> > > +     d = btf_dump__new(btf, NULL, NULL, btf_dump_printf);
> > > +     if (IS_ERR(d))
> > > +             return PTR_ERR(d);
> > > +
> > > +     if (root_type_cnt) {
> > > +             for (i = 0; i < root_type_cnt; i++) {
> > > +                     err = btf_dump__dump_type(d, root_type_ids[i]);
> > > +                     if (err)
> > > +                             goto done;
> > > +             }
> > > +     } else {
> > > +             int cnt = btf__get_nr_types(btf);
> > > +
> > > +             for (id = 1; id <= cnt; id++) {
> > > +                     err = btf_dump__dump_type(d, id);
> > > +                     if (err)
> > > +                             goto done;
> > > +             }
> > > +     }
> > > +
> > > +done:
> > > +     btf_dump__free(d);
> > > +     return err;  
> >
> > What do we do for JSON output?  
> 
> Still dump C syntax. What do you propose? Error out if json enabled?

I wonder.  Letting it just print C is going to confuse anything that
just feeds the output into a JSON parser.  I'd err on the side of
returning an error, we can always relax that later if we find a use
case of returning C syntax via JSON.

> > > +}
> > > +
> > >  static int do_dump(int argc, char **argv)
> > >  {
> > >       struct btf *btf = NULL;
> > >       __u32 root_type_ids[2];
> > >       int root_type_cnt = 0;
> > > +     bool dump_c = false;
> > >       __u32 btf_id = -1;
> > >       const char *src;
> > >       int fd = -1;
> > > @@ -431,6 +475,16 @@ static int do_dump(int argc, char **argv)
> > >               goto done;
> > >       }
> > >
> > > +     while (argc) {
> > > +             if (strcmp(*argv, "c") == 0) {
> > > +                     dump_c = true;
> > > +                     NEXT_ARG();
> > > +             } else {
> > > +                     p_err("unrecognized option: '%s'", *argv);
> > > +                     goto done;
> > > +             }
> > > +     }  
> >
> > This code should have checked there are no arguments and return an
> > error from the start :S  
> 
> I might be missing your point here. Lack of extra options is not an
> error, they are optional. It's just if there is an option, that we
> can't recognize - then we error out.

Oh, I was just remarking that before your patch bpftool would not error
if garbage options were passed, it'd be better if we errored from the
start.  But too late now, your code is good 👍

> > >       if (!btf) {
> > >               err = btf__get_from_id(btf_id, &btf);
> > >               if (err) {
> > > @@ -444,7 +498,10 @@ static int do_dump(int argc, char **argv)
> > >               }
> > >       }
> > >
> > > -     dump_btf_raw(btf, root_type_ids, root_type_cnt);
> > > +     if (dump_c)
> > > +             dump_btf_c(btf, root_type_ids, root_type_cnt);
> > > +     else
> > > +             dump_btf_raw(btf, root_type_ids, root_type_cnt);
> > >
> > >  done:
> > >       close(fd);
> > > @@ -460,7 +517,7 @@ static int do_help(int argc, char **argv)
> > >       }
> > >
> > >       fprintf(stderr,
> > > -             "Usage: %s btf dump BTF_SRC\n"
> > > +             "Usage: %s btf dump BTF_SRC [c]\n"  
> >
> > bpftool generally uses <key value> formats.  So perhaps we could do
> > something like "[format raw|c]" here for consistency, defaulting to raw?  
> 
> That's not true for options, though. I see that at cgroup, prog, and
> some map subcommands (haven't checked all other) just accept a list of
> options without extra identifying key.

Yeah, we weren't 100% enforcing this rule and it's a bit messy now :/

> > >               "       %s btf help\n"
> > >               "\n"
> > >               "       BTF_SRC := { id BTF_ID | prog PROG | map MAP [{key | value | kv | all}] | file FILE }\n"  

Powered by blists - more mailing lists