Date:	Mon, 6 Jan 2014 00:18:04 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Ivaylo Dimitrov <ivo.g.dimitrov.75@...il.com>
Cc:	gregkh@...uxfoundation.org, devel@...verdev.osuosl.org,
	linux-kernel@...r.kernel.org,
	Ivaylo Dimitrov <freemangordon@....bg>, pavel@....cz,
	pali.rohar@...il.com
Subject: Re: [PATCH] Staging: tidspbridge: Use hashtable implementation

This is not really an issue with this patch, it's something from the
original code so it doesn't affect merging the patch.

On Sun, Jan 05, 2014 at 08:55:47PM +0200, Ivaylo Dimitrov wrote:
> >Again, reverse the IS_ERR() check and return directly.  Use the struct
> >pointer instead of the address of the first member.
> >
> >	sym = gh_find(lib->sym_tab, (char *)name);
> >	if (IS_ERR(sym))
> >		return NULL;
> >
> >	return (struct dbll_symbol *)sym;
> >
> >That way is easier to read.  gh_find() should accept const pointers btw,
> >we shouldn't have to cast away the const in each of the callers.
> I don't think it is a good idea to return the structdbll_symbol*
> here - the function return type is structdynload_symbol* not
> structdbll_symbol*.

Oops.

> And that will break if we exchange value and name members of
> structdbll_symbol.

It will break anyway if we do that.  It's one of those things where you
can't worry too much about what crazy people might try to do later and
then no one reviews the code and no one tests it.  If we start doing
that sort of thing we are screwed already so it's not worth worrying
about.

I feel like the types in this driver are not beautiful.

The names are not clear.  Take "struct dbll_tar_obj" for example.  I'm
not clear what "dbll_" means.  I think the "d" stands for dynmic.  What
information does the "_obj" add?  It would be better to call it
"struct dbll_target".

"dbll_alloc" is a verb instead of a noun so it sounds like a function
not a struct.

But mostly there is too much nonsense casting throughout.
dbll_find_symbol() takes a struct dynamic_loader_sym pointer but we
immediatly cast it to struct ldr_symbol.  The struct ldr_symbol is
defined as:

struct ldr_symbol {
	struct dynamic_loader_sym dl_symbol;
        struct dbll_library_obj *lib;
};

The reason it does this is because it was originally C++ code and it
was ported to C.  I think we could move the "lib pointer to the end
of the "dynamic_loader_sym" struct.  We could make it a "void *data".
That would remove a source of casting.

There is a lot of this kind of stuff going on:

	struct dbll_library_obj *zl_lib = (struct dbll_library_obj *)lib;

"lib" is already a dbll_library_obj struct.  And "lib" is a better name,
what does zl_lib mean?  I think it's Hungarian notation which we don't
like.  Sometimes it uses pzl_ and I think the "p" means pointer.  Ugh.
Inside of functions then we don't need to prefix local variables.  It's
only for global variables where you run into naming clashes.

Anyway...  This driver needs a lot of fixing where data types are
concerned.

regards,
dan carpenter

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

Powered by blists - more mailing lists