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

[Pavel Simerda <psimerda redhat com>]

----- Original Message -----
> From: "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.

Renamed.

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

Added NM_LINK_TYPE_NONE, as this only happens in error condition.

Also added NM_LINK_TYPE_UNKNOWN to distinguish when kernel sends type information but we don't recognize it as a known type.

> - some code style issues, like { on the same line as the function
> name
> (eg, fake platform's link_get() function).

Fixed.

> 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));

I'd like to have a discussion whether it is really necessary to stick with this ugly alignment that IMO reduces readability and introduces unnecessarily long lines, e.g.:

g_ptr_array_index (some_object->some_attribute) = this_is_a_long_name_of_a_function (first_parameter,
<tens of spaces>second_parameter);

I consider this practice ugly and stupid. But will comply if I must :). One or two tab indents at the new line are much better.

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

Fixed. Removed obsolete conditional hack.

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

No big problem with that. Will talk about it later.

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

We can discuss that.

> - I'd say just assert if a subclass of NMPlatform doesn't implement
> required functions, instead of g_return_val_if_fail().

I wanted to avoid having to write empty functions just to get something work. IMO the tests are better in catching whether important things were implemented or not. And if I leave out something, I don't want the whole application to crash because of it, just to fail the single action.

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

That's what the tests are for, and the test coverage tools. We would have to test the existance of the function anyway, why not test their functionality too.

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

Done.

> - nm_platform_get_error_msg() should return 'const char *'

Done.

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

We can return -1 on error, if we consider zero a valid value (at least theoretically).

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

Why is the 'unsigned' so important? I thought enums were traditionally treated as signed integers (the default ones).

> - Just g_assert (platform) wherever you ahve
> g_return_if_fail(platform);

Sounds reasonable.

> if we don't have a platform, something is really, really wrong, and
> we might as well not try to hide it.

Done.

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

Done.

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

Done. I originally probably want to test it, but it's covered by the testsuite anyway.

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

Done.

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

This is libnl caching. We are using the libnl data structures and the libnl routines to fill them. The only thing we are not using is the cache manager.

The cache manager would replace part of the event processing, that's all. And to do proper synchronization between internal and external changes, we need to process them ourselves.

I discussed this with Thomas and we might be moving some of this functionality to libnl. Synchronization between internal and external changes is now not present at all.

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

There's no other caching. But we could move some of the code to libnl. But that's for a long discussion. Maybe in Brno?

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

The #defines should point to strings or to signal numbers?

> That's it for the moment...
> Dan

Pushed the resulting patch, will squash later.

Pavel