From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751348AbbEYNat (ORCPT ); Mon, 25 May 2015 09:30:49 -0400 Received: from mail.kernel.org ([198.145.29.136]:55245 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750813AbbEYNao (ORCPT ); Mon, 25 May 2015 09:30:44 -0400 Date: Mon, 25 May 2015 10:30:39 -0300 From: Arnaldo Carvalho de Melo To: Alexei Starovoitov Cc: Jiri Olsa , Wang Nan , paulus@samba.org, a.p.zijlstra@chello.nl, mingo@redhat.com, namhyung@kernel.org, jolsa@kernel.org, dsahern@gmail.com, daniel@iogearbox.net, brendan.d.gregg@gmail.com, masami.hiramatsu.pt@hitachi.com, lizefan@huawei.com, linux-kernel@vger.kernel.org, pi3orama@163.com Subject: Re: [RFC PATCH v3 09/37] bpf tools: Open eBPF object file and do basic validation Message-ID: <20150525133039.GD17970@kernel.org> References: <1431860222-61636-1-git-send-email-wangnan0@huawei.com> <1431860222-61636-10-git-send-email-wangnan0@huawei.com> <20150522172333.GE6609@krava.redhat.com> <555FD14A.1050908@plumgrid.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <555FD14A.1050908@plumgrid.com> X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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