linux-kernel - Re: [RFC PATCH v3 09/37] bpf tools: Open eBPF object file and do basic validation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150525133039.GD17970@kernel.org>
Date:	Mon, 25 May 2015 10:30:39 -0300
From:	Arnaldo Carvalho de Melo <acme@...nel.org>
To:	Alexei Starovoitov <ast@...mgrid.com>
Cc:	Jiri Olsa <jolsa@...hat.com>, Wang Nan <wangnan0@...wei.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

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/