Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751290AbbEZAGT (ORCPT ); Mon, 25 May 2015 20:06:19 -0400 Received: from szxga01-in.huawei.com ([58.251.152.64]:34379 "EHLO szxga01-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751045AbbEZAGP (ORCPT ); Mon, 25 May 2015 20:06:15 -0400 Message-ID: <5563B8C8.3090105@huawei.com> Date: Tue, 26 May 2015 08:05:28 +0800 From: "Wangnan (F)" User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Arnaldo Carvalho de Melo , Alexei Starovoitov CC: Jiri Olsa , , , , , , , , , , , , Subject: Re: [RFC PATCH v3 09/37] bpf tools: Open eBPF object file and do basic validation 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> <20150525133039.GD17970@kernel.org> In-Reply-To: <20150525133039.GD17970@kernel.org> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.111.66.109] X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4498 Lines: 126 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/