Re: [PATCH net-next 3/5] bpftool: implement cgattach command

2017-11-30 Thread Jakub Kicinski 
On Thu, 30 Nov 2017 13:43:00 +, Roman Gushchin wrote:
> + attach_type = parse_attach_type(argv[2]);
> + if (attach_type == __MAX_BPF_ATTACH_TYPE) {
> + bpf_object__close(obj);
> + close(prog_fd);
> + close(cgroup_fd);
> + p_err("Invalid attach type\n");
> + return -1;
> + }
> +
> + if (bpf_prog_attach(prog_fd, cgroup_fd, attach_type, 0)) {
> + bpf_object__close(obj);
> + close(prog_fd);
> + close(cgroup_fd);
> + p_err("Failed to attach program");
> + return -1;
> + }
> +
> + bpf_object__close(obj);
> + close(prog_fd);
> + close(cgroup_fd);
> +
> + return 0;
> +}

Could you try to consolidate the error paths into a one larger handler
and use gotos to jump to it?  You can see it done in number of places,
grep for e.g. exit_free.

Re: [PATCH net-next 3/5] bpftool: implement cgattach command

2017-11-30 Thread Roman Gushchin 
On Thu, Nov 30, 2017 at 09:17:17AM -0700, David Ahern wrote:
> On 11/30/17 6:43 AM, Roman Gushchin wrote:
> > @@ -75,12 +80,13 @@ static int do_help(int argc, char **argv)
> > fprintf(stderr,
> > "Usage: %s [OPTIONS] OBJECT { COMMAND | help }\n"
> > "   %s batch file FILE\n"
> > +   "   %s cgattach FILE CGROUP TYPE\n"
> 
> Can you change the order to:
> + "   %s cgattach CGROUP TYPE FILE\n"
> 
> Makes for better consistency with the detach command in the next patch:
> + "   %s cgdetach CGROUP TYPE ID\n"
> 
> 

Good point.

I'll fix this and will add support for attach_flags in v2.

Thanks!

Re: [PATCH net-next 3/5] bpftool: implement cgattach command

2017-11-30 Thread David Ahern 
On 11/30/17 6:43 AM, Roman Gushchin wrote:
> + if (bpf_prog_attach(prog_fd, cgroup_fd, attach_type, 0)) {
> + bpf_object__close(obj);
> + close(prog_fd);
> + close(cgroup_fd);
> + p_err("Failed to attach program");
> + return -1;
> + }


Also, what about support for BPF_F_ALLOW_OVERRIDE and BPF_F_ALLOW_MULTI
flags?

Re: [PATCH net-next 3/5] bpftool: implement cgattach command

2017-11-30 Thread David Ahern 
On 11/30/17 6:43 AM, Roman Gushchin wrote:
> @@ -75,12 +80,13 @@ static int do_help(int argc, char **argv)
>   fprintf(stderr,
>   "Usage: %s [OPTIONS] OBJECT { COMMAND | help }\n"
>   "   %s batch file FILE\n"
> + "   %s cgattach FILE CGROUP TYPE\n"

Can you change the order to:
+   "   %s cgattach CGROUP TYPE FILE\n"

Makes for better consistency with the detach command in the next patch:
+   "   %s cgdetach CGROUP TYPE ID\n"