On 2015/7/14 3:51, Arnaldo Carvalho de Melo wrote:
Em Fri, Jul 10, 2015 at 11:07:53AM +0800, Wangnan (F) escreveu:
On 2015/7/9 23:58, Arnaldo Carvalho de Melo wrote:
Em Thu, Jul 09, 2015 at 12:35:05PM +0000, Wang Nan escreveu:
This patch collects all programs in an object file into an array of
'struct bpf_program' for further processing. That structure is for
representing each eBPF program. 'bpf_prog' should be a better name, but
it has been used by linux/filter.h. Although it is a kernel space name,
I still prefer to call it 'bpf_program' to prevent possible confusion.

bpf_program__new() creates a new 'struct bpf_program' object. It first
init a variable in stack using __bpf_program__new(), then if success,
enlarges obj->programs array and copy the new object in.

Signed-off-by: Wang Nan <wangn...@huawei.com>
Acked-by: Alexei Starovoitov <a...@plumgrid.com>
Cc: Brendan Gregg <brendan.d.gr...@gmail.com>
Cc: Daniel Borkmann <dan...@iogearbox.net>
Cc: David Ahern <dsah...@gmail.com>
Cc: He Kuang <heku...@huawei.com>
Cc: Jiri Olsa <jo...@kernel.org>
Cc: Kaixu Xia <xiaka...@huawei.com>
Cc: Masami Hiramatsu <masami.hiramatsu...@hitachi.com>
Cc: Namhyung Kim <namhy...@kernel.org>
Cc: Paul Mackerras <pau...@samba.org>
Cc: Peter Zijlstra <a.p.zijls...@chello.nl>
Cc: Zefan Li <lize...@huawei.com>
Cc: pi3or...@163.com
Link: 
http://lkml.kernel.org/r/1435716878-189507-13-git-send-email-wangn...@huawei.com
Signed-off-by: Arnaldo Carvalho de Melo <a...@redhat.com>
---
  tools/lib/bpf/libbpf.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 117 insertions(+)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 9b016c0..3b717de 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -78,12 +78,27 @@ void libbpf_set_print(libbpf_print_fn_t warn,
  # define LIBBPF_ELF_C_READ_MMAP ELF_C_READ
  #endif
+/*
+ * bpf_prog should be a better name but it has been used in
+ * linux/filter.h.
+ */
+struct bpf_program {
+       /* Index in elf obj file, for relocation use. */
+       int idx;
+       char *section_name;
+       struct bpf_insn *insns;
+       size_t insns_cnt;
+};
+
  struct bpf_object {
        char license[64];
        u32 kern_version;
        void *maps_buf;
        size_t maps_buf_sz;
+       struct bpf_program *programs;
+       size_t nr_programs;
+
        /*
         * Information when doing elf related work. Only valid if fd
         * is valid.
@@ -100,6 +115,84 @@ struct bpf_object {
  };
  #define obj_elf_valid(o)      ((o)->efile.elf)
+static void bpf_program__clear(struct bpf_program *prog)
+{
+       if (!prog)
+               return;
+
+       zfree(&prog->section_name);
+       zfree(&prog->insns);
+       prog->insns_cnt = 0;
+       prog->idx = -1;
+}
So in perf land we use 'bpf_program__exit()' as the counterpart of
bpf_program__init(), i.e. one just initializes fields, allocating
memory for 'struct bpf_program' members, but does not allocates the
struct bpf_program itself, because sometimes we embed it inside other
structs, or we have it in arrays, as you do.

So, to keep that convention, please rename bpf_program__clear() to
bpf_program__exit() and the next function, __bpf_program__new() to
bpf_program__init(), with 'struct bpf_program *prog' as the first
parameter.

To speed things up, from now on, when I see such stuff, I will do the
changes, put them in a branch with a commiter note, and wait for your
Ack (or not, if you disagree with something).

One more comment below.

+
+static int
+__bpf_program__new(void *data, size_t size, char *name, int idx,
+                  struct bpf_program *prog)
+{
+       if (size < sizeof(struct bpf_insn)) {
+               pr_warning("corrupted section '%s'\n", name);
+               return -EINVAL;
+       }
+
+       bzero(prog, sizeof(*prog));
+
+       prog->section_name = strdup(name);
+       if (!prog->section_name) {
+               pr_warning("failed to alloc name for prog %s\n",
+                          name);
+               goto errout;
+       }
+
+       prog->insns = malloc(size);
+       if (!prog->insns) {
+               pr_warning("failed to alloc insns for %s\n", name);
+               goto errout;
+       }
+       prog->insns_cnt = size / sizeof(struct bpf_insn);
+       memcpy(prog->insns, data,
+              prog->insns_cnt * sizeof(struct bpf_insn));
+       prog->idx = idx;
+
+       return 0;
+errout:
+       bpf_program__clear(prog);
+       return -ENOMEM;
+}
+
+static struct bpf_program *
+bpf_program__new(struct bpf_object *obj, void *data, size_t size,
+                char *name, int idx)
This, as well, is not a 'bpf_program' method, it is a 'struct
bpf_object' one, that will manipulate 'struct bpf_object' internal
state, changing its struct members to get space for an extra bpf_program
that was initialized on the stack, if the initialization of such
bpf_program went well, or bail out otherwise.

So I suggest you rename this to:

int bpf_object__add_program(struct bpf_object *obj, void *data, size_t size, 
char *name, int idx)

And probably move that debug that uses prog->section_name to just after
the realloc, here in this function.

I will look at the other patches after lunch, thanks for providing the
git tree, I will try and use it before looking at the patches
individually, to get a feel of the whole thing.
I didn't find your code, so I updated my git tree. Please see:

  
https://github.com/WangNan0/linux/commit/e5ffa4f070ee36cce5130d08622dc305ad9cdb31
Ok, so used bpf_object__add_program, but you still return a bpf_program
pointer, that you do not use for anything, i.e. the failure of
bpf_object__add_program is reported only via a NULL return and you then
assume this was because ENOMEM was the reason, when there are multiple
errors that can cause bpf_object__add_program to fail.

Noted that with a comment on that patch, checked that no later patches
use that return, etc.

I saw your modification ann it looks good to me. I'll collect it into my patchset.

Thank you.

- Arnaldo


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Reply via email to