Re: public status report on nm-platform and enterprise networking

[Dan Williams <dcbw redhat com>]

On Wed, 2012-12-05 at 12:15 -0500, Pavel Simerda wrote:
> Hi,
> 
> I've been working on the 'pavlix/platform' branch for some time already. Now I feel it is a time to make a public comment. Meanwhile Dan Williams merged the long-awaited 'bridge' branch. So I rebased
> 'pavlix/platform' on top of the new master.

Initial platform branch review notes...

- I'd rather have NM_LINK_TYPE_GENERIC refer to non-bond/vlan/bridge
interfaces; it's not that the link is unknown, it's just a generic
interface that doesn't have any special functionality.

- But keep NM_LINK_TYPE_UNKNOWN and use that as the return value for
nm_platform_link_get_type() instead of int.  That way we can just use 
"if (!link_type) { error stuff }" instead of "if (link_type < 0) { error
stuff }"; which is clearer because 0 is more often an error/NULL
condition than -1 is.

- some code style issues, like { on the same line as the function name
(eg, fake platform's link_get() function).  Also, you're using tabs for
spacing, instead of spaces, like in linux link_release():

		debug ("(%s): Could not release slave %s: %d (%s)",
			link_index_to_name (platform, master),
			link_index_to_name (platform, slave),
			result, nl_geterror (result));

this is not going to align correctly in any editor unless your tabs are
set to be 7 spaces wide.  Should be:

<tab><tab>debug ("(%s): Could not release slave %s: %d (%s)",
<tab><tab><spc*7>link_index_to_name (platform, master),
<tab><tab><spc*7>link_index_to_name (platform, slave),
<tab><tab><spc*7>result, nl_geterror (result));

- missing {} in socket_setup() around the "if (event)" statement

- I'm still not convinced about the error handling the way it is; IMHO
it's a lot clearer (and easier to push the errors up to callers) to do
the GError style of things.  Also, that means you can send a descriptive
string along with the error that indicates the *specific* thing that
failed, which the generic num -> string conversion cannot do.  Eg, think
about open(2): it just returns an error code, but no information about
the file that you tried to open.  So if the open(2) was buried in the
stack, when the error finally pops up to the original caller of some
library function, you have no idea what happened, just ENOENT.  It's
nice to see a more detailed error like G_ERROR_FILE_NOT_FOUND + "The
file /foo/bar/baz.txt did not exist".  GError really isn't that hard to
do, nor does it clutter up the code much; I believe it's a win in the
long run.  I'm happy to help fix up the platform stuff to add it if
you'd rather spend time on other things.

- Might want to look into using a model like GHashTableIter for the
nm_platform_link_foreach() method; it's often quite annoying, and more
code, to create a small "context" structure, and then a whole new
function for each thing iterated.  With the GHashTableIter model, you
can do all that in one function and make things simpler and more
concise.

- I'd say just assert if a subclass of NMPlatform doesn't implement
required functions, instead of g_return_val_if_fail().  We know the
subclasses need to implement some of this stuff no matter what, so we
might as well make sure that's the case instead of covering it up.

- set_address() argument types should be harmonized, platform has
"gconstpointer" while linux platform uses "const void *address".  SHould
pick one or the other

- nm_platform_get_error_msg() should return 'const char *'

- nm_platform_link_count() should return guint32, not int.  We'll never,
ever have -1 links

- nm_platform_link_get_type() as above, lets return guint32 and use
NM_LINK_TYPE_UNKNOWN to indicate an error

- Just g_assert (platform) wherever you ahve g_return_if_fail(platform);
if we don't have a platform, something is really, really wrong, and we
might as well not try to hide it.

- Code style, ! has a space after it in fake platform's link_delete():

	if (idx <= 0 || idx >= priv->links->len ||! g_ptr_array_index
(priv->links, idx)) {

- fake platform link_name_to_index(): any reason not to just use a for
(i = 0; priv->links && i < priv->links->len; i++) of using
g_ptr_array_foreach?  That's a lot less code.

- I noticed a few places you can use g_strcmp0() instead of just
strcmp(); not handling NULL in strcmp is about the dumbest thing ever in
the history of the universe for 99% of cases.  eg fake find_link(),
linux link_extract_type(), and in the future.

- Given that the Linux platform caches a lot of info instead of asking
libnl for each when it's needed, is there a chance the kernel may not
tell us when things change and thus our cache will be out of date?
Also, are we caching this stuff internally because libnl's caching is
broken?  Can we drop some of our internal caching when libnl's gets
fixed, if it's broken?

- Also, wherever you have signal names, for example linux
link_extract_type(), lets #define those.  Quite a few times over the
years I've found that I've mis-spelled a signal here or there, and you
only find that out at runtime.  Much better to make it a compile-time
error instead.

That's it for the moment...
Dan