Date: Mon, 25 May 2015 10:30:39 -0300
From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Alexei Starovoitov <ast@plumgrid.com>
Cc: Jiri Olsa <jolsa@redhat.com>, Wang Nan <wangnan0@huawei.com>,
        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>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3789
Lines: 115

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
--
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/