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

[Arnaldo Carvalho de Melo]

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