[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5563B8C8.3090105@huawei.com>
Date: Tue, 26 May 2015 08:05:28 +0800
From: "Wangnan (F)" <wangnan0@...wei.com>
To: Arnaldo Carvalho de Melo <acme@...nel.org>,
Alexei Starovoitov <ast@...mgrid.com>
CC: Jiri Olsa <jolsa@...hat.com>, <paulus@...ba.org>,
<a.p.zijlstra@...llo.nl>, <mingo@...hat.com>,
<namhyung@...nel.org>, <jolsa@...nel.org>, <dsahern@...il.com>,
<daniel@...earbox.net>, <brendan.d.gregg@...il.com>,
<masami.hiramatsu.pt@...achi.com>, <lizefan@...wei.com>,
<linux-kernel@...r.kernel.org>, <pi3orama@....com>
Subject: Re: [RFC PATCH v3 09/37] bpf tools: Open eBPF object file and do
basic validation
On 2015/5/25 21:30, Arnaldo Carvalho de Melo wrote:
> Em Fri, May 22, 2015 at 06:00:58PM -0700, Alexei Starovoitov escreveu:
>> On 5/22/15 10:23 AM, Jiri Olsa wrote:
>>>> +struct bpf_object *bpf_open_object(const char *path)
>>> another suggestion for the namespace.. Arnaldo forces us ;-)
>>> to use the object name first plus '__(method name)' for
>>> interface functions so that would be:
>>> bpf_object__open
>>> bpf_object__close
>>> not sure we want to keep that standard in here though.. Arnaldo?
>
>> have been thinking back and forth on this one.
>> Finally convinced myself that we shouldn't be forcing it here.
>> object__method style would force the library to look like fake
>> object oriented whereas it's not. It's a normal C. Let's keep it
> Why "fake"? Just because C doesn't have explicit support for OO doesn't
> mean we can't use the concept of OO with structs and functions :-)
>
>> simple. Objects are not needed here. May be 'bpf_object' is an
>> unfortunate name, but it doesn't make the library to be 'ooo'.
> Well, I don't think that what leads one to think about using some
> convention was because "object" was in its name, but because OO _is_
> being used in this case, albeit a restricted set, the one possible while
> using C.
>
> For instance, in this patch:
>
> struct bpf_object {
> /*
> * Information when doing elf related work. Only valid if fd
> * is valid.
> */
> struct {
> int fd;
> Elf *elf;
> GElf_Ehdr ehdr;
> } elf;
> char path[]; /* Changed from being a pointer to here, to avoid one alloc */
> };
>
> static struct bpf_object *__bpf_obj_alloc(const char *path)
> {
> struct bpf_object *obj;
>
> obj = calloc(1, sizeof(struct bpf_object));
> if (!obj) {
> pr_warning("alloc memory failed for %s\n", path);
> return NULL;
> }
>
> obj->path = strdup(path);
> if (!obj->path) {
> pr_warning("failed to strdup '%s'\n", path);
> free(obj);
> return NULL;
> }
> return obj;
> }
>
> The above is for me naturally a constructor, in the restricted OO
> possible with C used in tools/perf (or anywhere else :)), and thus we
> have a convention for this, short one, struct being instantiated + __ +
> new.
>
> struct bpf_object *bpf_object__new(const char *path)
> {
> struct bpf_object *obj = zalloc(sizeof(*obj) + strlen(path) + 1);
>
> if (obj) {
> strcpy(obj->path, path);
> obj->elf.fd = -1;
> }
>
> return obj;
> }
>
> ---
>
> If it doesn't allocates, i.e. it is embedded in another struct, then, we
> have struct being initiated + __ + init, and so on for __delete +
> __exit, etc.
>
> Just a convention, that we (or at least I) try to follow as judiciously
> as possible in tools/perf/.
>
> Like others in the kernel, like using, hey, "__" in front of functions
> to state that they do slightly less than the a function with the same
> name, normally locking is done on foo() that calls __foo() to do the
> unlocked part.
>
> It avoids ambiguity as what is the struct being acted upon by the
> function, since we use _ to separate words in the struct name
> (bpf_object, perf_evlist, etc) and in the function name (findnew_thread,
> process_events, etc), helps with grepping the source code base, etc.
>
>> libtraceevent doesn't use this style either...
> Well, there are many styles to pick, the fact that perf uses __ to
> separate class name from class method doesn't mean that you should as
> well, as you may find it inconvenient or useless to you, you may prefer
> CamelCase notation, for instance ;-)
>
> In the same fashion the fact that libtraceevent doesn't doesn't mean you
> shouldn't use what the perf tooling uses.
>
> - Arnaldo
I'll try OO style naming and see the results in my next version.
However, I'm
not very sure whether such naming make sense, since we have only 2
classes in
libbpf: 'bpf_object, bpf_program', 'bpf_map' has potential to become a
class but
currently not. In addition, the internal functions are hidden to user,
so the only
meaning to user of such API is an additional '_' in each function.
Anyway, let's see the result then decide whether it is good enough.
Thank you.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists