Patches can be found in branch 'pavlix/platform'. Comments welcome.

The following command can be used to check the netlink/nm-netlink dependencies:

grep -ri '#include.*netlink' src | grep -vE '^src/(nm-netlink|platform/)'

For practical purposes (too many code dependencies), the NMPlatform patches can
now be found in 'pavlix/next' together with a bunch of other patches.

It is not yet ready to merge/rebase. Even its history will be repeatedly rewritten. Use it only for your testing and always git-fetch & git-reset,
not git-pull.

Part of this feature is under review (in pavlix/nm-platform-link branch).

Looks good in general.


If we're requiring libnl-3, we might want to put in a check to block wimax-linked-against-libnl-1 (bug 687630). And we definitely want to make sure something ends up in NEWS pointing out where to get the forked wimax libs.

Also, we should remove all the nl-1 and nl-2 compat code... Maybe all that should happen as a separate patch before the platform patch.


Though apparently not needed for this patch, you should add platform, platform/fake and platform/linux to the lists in src/generated/Makefile.am, so if we add things later that need generated code, they'll get generated correctly.


src/logging/nm-logging.c:
>-	{ LOGD_HW,        "HW" },
>+	{ LOGD_PLATFORM,    "PLATFORM" },
> 	{ LOGD_RFKILL,    "RFKILL" },

might as well keep that aligned


src/platform/nm-platform.h:
>+/* NMPlatform error codes */
>+enum {

It's more stylistically consistent to typedef the enum (eg, "NMPlatformError"), and use that type rather than "int" in the appropriate places. Likewise for the link types.

And really, it would be more idiomatic to have the NMPlatform methods use GError, rather than having a separate nm_platform_get_error() and nm_platform_get_error_code().



src/platform/nm-platform.c:

>+ * nm_platform_setup:
>+ * @type: The #GType for a subclass of #NMPlatform
>+ *
>+ * Do not use this function directly, it is intended to be called by
>+ * NMPlatform subclasses. For the linux platform initialization use
>+ * nm_platform_linux_setup() instead.

Why? Why not just have people call nm_platform_setup(NM_TYPE_LINUX_PLATFORM)?

(Also, the docs get the name wrong: "nm_linux_platform_setup()", not "nm_platform_linux_setup()".)

Also, rather than having a setup() method, you could have the platform objects implement GInitable, and then call g_initable_init() on them rather than nm_platform_setup(). Shrug.


>+	g_assert (klass);
>+	g_return_if_fail (platform);
etc

Minor style point, but normally assertions/return-if-fails are always explicitly "foo == NULL" or "foo != NULL", because "assertion failed: platform" is sort of mysterious, while "assertion failed: platform != NULL" is clear.


>+nm_platform_link_foreach (NMLinkCallback callback, gpointer user_data)

hard to say for sure without seeing how it's going to be used, but normally foreach functions are a pain. Would it be simpler to just have "nm_platform_get_links()" (returning NMPlatformLink** or GPtrArray*)?


>+ * nm_platform_link_get_type:
>+ * @idx: Interface index.
>+ *
>+ * Returns: Link type constant as defined in nm-platform.h.

what about on error?


>+ * nm_platform_link_name_to_index:
>+ * @name: Interface name
>+ *
>+ * Returns: The interface index corresponding to the given interface name
>+ * or -1. Inteface name is owned by #NMPlatform, don't free it.

nm_platform_link_name_to_index() and nm_platform_link_index_to_name() have exactly the same documentation, part of which only makes sense for name_to_index, and part of which only makes sense for index_to_name... (and "Interface" is misspelled in the last sentence)

Also, if you're not supposed to free the return value of the index_to_name(), just make it const, and then you don't need to explicitly document that.

Although... NMLinuxPlatform returns a newly-allocated string every time you call index_to_name(), whereas NMFakePlatform does not... so maybe you started to change the semantics and didn't finish? Anyway, if you want to stick with the "const" behavior, you can have NMLinuxPlatform read the name from libnl into a "char ifname[IFNAMSIZ]", and then "return g_intern_string(ifname)".


>+nm_platform_link_is_up (int idx)
>+nm_platform_link_is_connected (int idx)

Not clear what the difference between these two is from the names/docs


>+nm_platform_link_is_arp (int idx)

"is ARP" is weird. "nm_platform_link_uses_arp" maybe? "nm_platform_link_arp_is_enabled"?

>+nm_platform_link_set_arp (int idx)
>+nm_platform_link_set_noarp (int idx)

Again, hard to say for sure without seeing it in use, but would it make more sense to have a single method that takes a gboolean (like we have now in nm-system)?

Likewise, possibly, with set_up/set_down.


>+ * nm_platform_link_get_mtu:
>+ * @idx: Interface index
>+ *
>+ * Returns: MTU value for the interface.

or 0 on error I guess? (Actually, the return-if-fails return FALSE, which isn't quite right...)


>+ * nm_platform_link_get_address:
>+ * @idx: Interface index
>+ * @lenght: Pointer to a variable to store address length (mandatory)
>+ *
>+ * Saves interface MAC address to @address.

typo in "@lenght", and I'd say "hardware address" rather than "MAC address", since "MAC" is technically incorrect at least for InfiniBand.


>+nm_platform_link_enslave (int master, int slave)

rtnl_link_enslave_ifindex() is actually a bond-specific operation; bridges use a different API (which isn't currently netlink-able). So anyway, the name should indicate this (and we'll need another pair of methods for bridges). The corresponding nm-system method was recently renamed to "nm_system_bond_enslave()" (and likewise "nm_system_bond_release()"). Although "add/remove_bond_slave()" sounds better to me...

Alternatively, you could say that the platform implementation has to check the link type of @master and then use the right lower-level API from there. Which might make sense, since otherwise we'll need yet another pair of methods for teams...


>+nm_platform_bridge_add (const char *name)
>+nm_platform_bond_add (const char *name)
>+nm_platform_vlan_add (const char *name, int parent, int vlanid, guint32 vlanflags)

It would probably be nice to have these return the link index of the newly-created device?


>+gboolean
>+nm_platform_vlan_get_info (int idx, int *parent, int *vlanid)

I'd expect that this would return FALSE if @idx isn't a VLAN, but neither backend does that.




src/platform/fake/nm-fake-platform.c:

>+link_delete (NMPlatform *platform, int idx)

need to free device->name and maybe device->address


>+link_name_to_index (NMPlatform *platform, const char *name)

>+		if (device && device->name && !g_strcmp0 (device->name, name))

don't need to check that device->name is non-NULL if you're using g_strcmp0.


>+link_get_mtu (NMPlatform *platform, int idx)
(and a few others)

>+	g_return_val_if_fail (device, FALSE);

that's inconsistent with the other methods, which return an error code without return-if-failing if idx is bad.


>+link_set_address (NMPlatform *platform, int idx, gconstpointer address, size_t length)

need to free the existing address if it's set


>+link_add (NMPlatform *platform, int type, const char *name)

>+	device = link_new (priv->links->len, type, name);
>+
>+	g_ptr_array_add (priv->links, type >= 0 ? device : NULL);
>+	g_signal_emit_by_name (platform, "link-added", device);

this leaks device if type == 0, and emits a link-added signal for a device that will later not be visible.


>+nm_fake_platform_init (NMFakePlatform *fake_platform)

>+	priv->links = g_ptr_array_new ();

use g_ptr_array_new_with_free_func() to avoid leaking (or else do cleanup from finalize)




src/platform/linux/nm-linux-platform.c:

>+link_refresh (NMPlatform *platform, int idx, const char *name)

>+	if (cached_object) {
>+		nl_cache_remove (cached_object);
>+		g_return_val_if_fail (nl_cache_add (priv->link_cache, kernel_object) == 0, FALSE);
>+		g_signal_emit_by_name (platform, "link-changed", &info);
>+	} else {
>+		g_return_val_if_fail (nl_cache_add (priv->link_cache, kernel_object) == 0, FALSE);
>+		g_signal_emit_by_name (platform, "link-added", &info);
>+	}

You have to do

    status = nl_cache_add (...);
    g_return_val_if_fail (status == 0, FALSE);

because g_return_val_if_fail(x) expands to nothing if compiled with -DG_DISABLE_CHECKS.


>+link_get_address (NMPlatform *platform, int idx, size_t *length)

>+	result = malloc (alen);
>+	/* TODO check */

check what? That malloc didn't fail? No, just use g_malloc().

Except, that this isn't supposed to return allocated memory. Or at least, the nm_platform_link_get_address() docs don't say you have to free the return value, and the fake implementation returns its internal address rather than making a copy. So, something needs fixing here.

One possibility is to require the caller to pass in a buffer; they ought to already know how big it needs to be, based on the device type (and can always just make it be NM_UTILS_HWADDR_LEN_MAX if not).


>+process_link_change (NMPlatform *platform,

>+		g_signal_emit_by_name (platform, "link-removed", &info);

info hasn't been filled in at this point


>+emit_link_change (NMPlatform *platform, const char *sig, struct nl_object *object)

>+	g_message ("%s", sig);

leftover debugging?

likewise, there's a bunch of commented-out stuff in event_notification()


>+verify_source (struct nl_msg *msg, gpointer user_data)

>+	/* Ignore message with non-zero port number (message from another application) */
>+	if (src->nl_pid != 0) {

"non-zero pid" not "non-zero port"


>+setup_socket (gboolean event, gpointer user_data)

>+#ifdef LIBNL_NEEDS_ADDR_CACHING_WORKAROUND
>+	nl_cache *addr_cache;
>+#endif

cruft


>+setup (NMPlatform *platform)

>+	/*if (nl_socket_add_membership (priv->nlh_event, RTNLGRP_LINK)) {
>+		debug ("Failed to join netlink multicast group.");
>+		return FALSE;
>+	}*/

seems entirely obsoleted by the nl_socket_add_memberships() call that follows?

>+	g_return_val_if_fail (g_io_channel_set_flags (priv->event_channel,
>+	                        channel_flags | G_IO_FLAG_NONBLOCK, NULL), FALSE);

as before, need to do that outside the return-if-fail. (And likewise with the cache allocations below.)

(In reply to comment #5)
> Looks good in general.

Thanks.

> If we're requiring libnl-3, we might want to put in a check to block
> wimax-linked-against-libnl-1 (bug 687630).

I was already thinking about this but have never done a check like that. Would you use ldd for that?

> And we definitely want to make sure
> something ends up in NEWS pointing out where to get the forked wimax libs.

+1

> Also, we should remove all the nl-1 and nl-2 compat code... Maybe all that
> should happen as a separate patch before the platform patch.

It is already removed in the platform directory. When we're done with nm-platform integration, the whole nm-netlink-* stuff will go away.

> Though apparently not needed for this patch, you should add platform,
> platform/fake and platform/linux to the lists in src/generated/Makefile.am, so
> if we add things later that need generated code, they'll get generated
> correctly.

Could be. I tend to ignore directories I don't need but if you think this is a good idea...

> src/logging/nm-logging.c:
> >-	{ LOGD_HW,        "HW" },
> >+	{ LOGD_PLATFORM,    "PLATFORM" },
> > 	{ LOGD_RFKILL,    "RFKILL" },
> 
> might as well keep that aligned

Ok.
 
> src/platform/nm-platform.h:
> >+/* NMPlatform error codes */
> >+enum {
> 
> It's more stylistically consistent to typedef the enum (eg, "NMPlatformError"),
> and use that type rather than "int" in the appropriate places. Likewise for the
> link types.

Ok.

> And really, it would be more idiomatic to have the NMPlatform methods use
> GError, rather than having a separate nm_platform_get_error() and
> nm_platform_get_error_code().

Transition to GError is already agreed upon. This can be done before or after merge, whatever is preferred.

dcbw offered he could possibly do that but we didn't talk about it since then.

> src/platform/nm-platform.c:
> 
> >+ * nm_platform_setup:
> >+ * @type: The #GType for a subclass of #NMPlatform
> >+ *
> >+ * Do not use this function directly, it is intended to be called by
> >+ * NMPlatform subclasses. For the linux platform initialization use
> >+ * nm_platform_linux_setup() instead.
> 
> Why? Why not just have people call nm_platform_setup(NM_TYPE_LINUX_PLATFORM)?

I wanted one preferred way and a setup function seemed more natural to me. I don't really care which one is it.

> (Also, the docs get the name wrong: "nm_linux_platform_setup()", not
> "nm_platform_linux_setup()".)

Thx.

> Also, rather than having a setup() method, you could have the platform objects
> implement GInitable, and then call g_initable_init() on them rather than
> nm_platform_setup(). Shrug.

Could be. As with GError, I don't see any direct benefits for this specific case except exercising GError and GInitable, but I'm not against that.

> >+	g_assert (klass);
> >+	g_return_if_fail (platform);
> etc
> 
> Minor style point, but normally assertions/return-if-fails are always
> explicitly "foo == NULL" or "foo != NULL", because "assertion failed: platform"
> is sort of mysterious,

When I see assertion failed 'platform', I immediately know that platform is empty/false/nonexistent. If you insist, I'll change that.

> while "assertion failed: platform != NULL" is clear.
> 
> 
> >+nm_platform_link_foreach (NMLinkCallback callback, gpointer user_data)
> 
> hard to say for sure without seeing how it's going to be used, but normally
> foreach functions are a pain.

There are to reasons:

1) This closely follows the original API

2) It avoids having to allocate and dealocate the objects as they are created on the fly from some internal representation. That's because we're using nl_cache internally.

Of course that's not the only possible way and could be changed, possibly after merging.

> Would it be simpler to just have
> "nm_platform_get_links()" (returning NMPlatformLink** or GPtrArray*)?

The callback method allocates one NMPlatformLink structure on the stack, the other one would require may allocations and deallocations on the heap. But yes, it's possible.

> >+ * nm_platform_link_get_type:
> >+ * @idx: Interface index.
> >+ *
> >+ * Returns: Link type constant as defined in nm-platform.h.
> 
> what about on error?

Ok.

> >+ * nm_platform_link_name_to_index:
> >+ * @name: Interface name
> >+ *
> >+ * Returns: The interface index corresponding to the given interface name
> >+ * or -1. Inteface name is owned by #NMPlatform, don't free it.
> 
> nm_platform_link_name_to_index() and nm_platform_link_index_to_name() have
> exactly the same documentation, part of which only makes sense for
> name_to_index, and part of which only makes sense for index_to_name... (and
> "Interface" is misspelled in the last sentence)

Thx.

> Also, if you're not supposed to free the return value of the index_to_name(),
> just make it const, and then you don't need to explicitly document that.

Ok.

> Although... NMLinuxPlatform returns a newly-allocated string every time you
> call index_to_name(), whereas NMFakePlatform does not... so maybe you started
> to change the semantics and didn't finish? Anyway, if you want to stick with
> the "const" behavior, you can have NMLinuxPlatform read the name from libnl
> into a "char ifname[IFNAMSIZ]", and then "return g_intern_string(ifname)".

Good point. I'm not sure if I like the g_intern_string for interface names but let's discuss it later.

> >+nm_platform_link_is_up (int idx)
> >+nm_platform_link_is_connected (int idx)
> 
> Not clear what the difference between these two is from the names/docs

Ok.

> >+nm_platform_link_is_arp (int idx)
> 
> "is ARP" is weird. "nm_platform_link_uses_arp" maybe?
> "nm_platform_link_arp_is_enabled"?

Ok.

> >+nm_platform_link_set_arp (int idx)
> >+nm_platform_link_set_noarp (int idx)
> 
> Again, hard to say for sure without seeing it in use,

Only nm_platform_link_set_noarp (idx) is used for some mobile devices. The set_arp and uses_arp functions are only used by the testsuite.

> but would it make more
> sense to have a single method that takes a gboolean (like we have now in
> nm-system)?

Was trying to keep it consistent with up/down.

> Likewise, possibly, with set_up/set_down.

These are distinct actions, done often in different situations. And nm_platform_set_up (..., FALSE) looks weird.

> >+ * nm_platform_link_get_mtu:
> >+ * @idx: Interface index
> >+ *
> >+ * Returns: MTU value for the interface.
> 
> or 0 on error I guess? (Actually, the return-if-fails return FALSE, which isn't
> quite right...)

Thx.

> >+ * nm_platform_link_get_address:
> >+ * @idx: Interface index
> >+ * @lenght: Pointer to a variable to store address length (mandatory)
> >+ *
> >+ * Saves interface MAC address to @address.
> 
> typo in "@lenght", and I'd say "hardware address" rather than "MAC address",
> since "MAC" is technically incorrect at least for InfiniBand.

Ok.

> >+nm_platform_link_enslave (int master, int slave)
> 
> rtnl_link_enslave_ifindex() is actually a bond-specific operation; bridges use
> a different API (which isn't currently netlink-able).

It works for both bridges and bonds, at least with kernels I tested. Run src/platform/tests/nm-linux-platform-test on a system you suspect might not support it.

> So anyway, the name
> should indicate this (and we'll need another pair of methods for bridges). The
> corresponding nm-system method was recently renamed to
> "nm_system_bond_enslave()" (and likewise "nm_system_bond_release()"). Although
> "add/remove_bond_slave()" sounds better to me...
> 
> Alternatively, you could say that the platform implementation has to check the
> link type of @master and then use the right lower-level API from there. Which
> might make sense, since otherwise we'll need yet another pair of methods for
> teams...
> 
> 
> >+nm_platform_bridge_add (const char *name)
> >+nm_platform_bond_add (const char *name)
> >+nm_platform_vlan_add (const char *name, int parent, int vlanid, guint32 vlanflags)
> 
> It would probably be nice to have these return the link index of the
> newly-created device?

Looks like a good idea.

> >+gboolean
> >+nm_platform_vlan_get_info (int idx, int *parent, int *vlanid)
> 
> I'd expect that this would return FALSE if @idx isn't a VLAN, but neither
> backend does that.

Done in the parent class.

> src/platform/fake/nm-fake-platform.c:
> 
> >+link_delete (NMPlatform *platform, int idx)
> 
> need to free device->name and maybe device->address

Ok.

> >+link_name_to_index (NMPlatform *platform, const char *name)
> 
> >+		if (device && device->name && !g_strcmp0 (device->name, name))
> 
> don't need to check that device->name is non-NULL if you're using g_strcmp0.

Ok.

> >+link_get_mtu (NMPlatform *platform, int idx)
> (and a few others)
> 
> >+	g_return_val_if_fail (device, FALSE);
> 
> that's inconsistent with the other methods, which return an error code without
> return-if-failing if idx is bad.

Index should never be wrong, under normal circumstances. 

> >+link_set_address (NMPlatform *platform, int idx, gconstpointer address, size_t length)
> 
> need to free the existing address if it's set

Ok.

> >+link_add (NMPlatform *platform, int type, const char *name)
> 
> >+	device = link_new (priv->links->len, type, name);
> >+
> >+	g_ptr_array_add (priv->links, type >= 0 ? device : NULL);
> >+	g_signal_emit_by_name (platform, "link-added", device);
> 
> this leaks device if type == 0, and emits a link-added signal for a device that
> will later not be visible.

Ok.

> >+nm_fake_platform_init (NMFakePlatform *fake_platform)
> 
> >+	priv->links = g_ptr_array_new ();
> 
> use g_ptr_array_new_with_free_func() to avoid leaking (or else do cleanup from
> finalize)

Ok.

> src/platform/linux/nm-linux-platform.c:
> 
> >+link_refresh (NMPlatform *platform, int idx, const char *name)
> 
> >+	if (cached_object) {
> >+		nl_cache_remove (cached_object);
> >+		g_return_val_if_fail (nl_cache_add (priv->link_cache, kernel_object) == 0, FALSE);
> >+		g_signal_emit_by_name (platform, "link-changed", &info);
> >+	} else {
> >+		g_return_val_if_fail (nl_cache_add (priv->link_cache, kernel_object) == 0, FALSE);
> >+		g_signal_emit_by_name (platform, "link-added", &info);
> >+	}
> 
> You have to do
> 
>     status = nl_cache_add (...);
>     g_return_val_if_fail (status == 0, FALSE);
> 
> because g_return_val_if_fail(x) expands to nothing if compiled with
> -DG_DISABLE_CHECKS.

Thx.

> >+link_get_address (NMPlatform *platform, int idx, size_t *length)
> 
> >+	result = malloc (alen);
> >+	/* TODO check */
> 
> check what? That malloc didn't fail? No, just use g_malloc().

Ok.

> Except, that this isn't supposed to return allocated memory. Or at least, the
> nm_platform_link_get_address() docs don't say you have to free the return
> value, and the fake implementation returns its internal address rather than
> making a copy. So, something needs fixing here.

Ok.

> One possibility is to require the caller to pass in a buffer; they ought to
> already know how big it needs to be, based on the device type (and can always
> just make it be NM_UTILS_HWADDR_LEN_MAX if not).

Looks ugly. What about just returning const address and letting the caller copy it if it needs to keep it?

> >+process_link_change (NMPlatform *platform,
> 
> >+		g_signal_emit_by_name (platform, "link-removed", &info);
> 
> info hasn't been filled in at this point

Hmm.

> >+emit_link_change (NMPlatform *platform, const char *sig, struct nl_object *object)
> 
> >+	g_message ("%s", sig);
> 
> leftover debugging?

Probably.

> likewise, there's a bunch of commented-out stuff in event_notification()

Ok.

> >+verify_source (struct nl_msg *msg, gpointer user_data)
> 
> >+	/* Ignore message with non-zero port number (message from another application) */
> >+	if (src->nl_pid != 0) {
> 
> "non-zero pid" not "non-zero port"

Ok.

> >+setup_socket (gboolean event, gpointer user_data)
> 
> >+#ifdef LIBNL_NEEDS_ADDR_CACHING_WORKAROUND
> >+	nl_cache *addr_cache;
> >+#endif
> 
> cruft

Yep.

> >+setup (NMPlatform *platform)
> 
> >+	/*if (nl_socket_add_membership (priv->nlh_event, RTNLGRP_LINK)) {
> >+		debug ("Failed to join netlink multicast group.");
> >+		return FALSE;
> >+	}*/
> 
> seems entirely obsoleted by the nl_socket_add_memberships() call that follows?

Yes.

> >+	g_return_val_if_fail (g_io_channel_set_flags (priv->event_channel,
> >+	                        channel_flags | G_IO_FLAG_NONBLOCK, NULL), FALSE);
> 
> as before, need to do that outside the return-if-fail. (And likewise with the
> cache allocations below.)

Ok.

(In reply to comment #6)
> I was already thinking about this but have never done a check like that. Would
> you use ldd for that?

not really sure...

> When I see assertion failed 'platform', I immediately know that platform is
> empty/false/nonexistent. If you insist, I'll change that.

up to dcbw

> Good point. I'm not sure if I like the g_intern_string for interface names but
> let's discuss it later.

It seems like a pretty good match; the number of unique interface names NM will see during a given run is fairly small. (Eg, it's probably less than the total number of GObject types used by NM, each of which has an interned name.)

> Only nm_platform_link_set_noarp (idx) is used for some mobile devices. The
> set_arp and uses_arp functions are only used by the testsuite.

OK, then yeah, I guess separate noarp() makes sense.

> > Likewise, possibly, with set_up/set_down.
> 
> These are distinct actions, done often in different situations. And
> nm_platform_set_up (..., FALSE) looks weird.

Yeah, that's why I said "possibly" for that one. Separate up/down already made more sense to me than separate arp/noarp.

> > rtnl_link_enslave_ifindex() is actually a bond-specific operation; bridges use
> > a different API (which isn't currently netlink-able).
> 
> It works for both bridges and bonds, at least with kernels I tested.

Hm... OK. I wonder why Thomas didn't do that originally then? Maybe just to avoid having separate nl3 and fallback codepaths I guess...

> > >+	g_return_val_if_fail (device, FALSE);
> > 
> > that's inconsistent with the other methods, which return an error code without
> > return-if-failing if idx is bad.
> 
> Index should never be wrong, under normal circumstances. 

OK, but lots of methods just return NL_PLATFORM_ERROR_NOTFOUND in that case...

> > One possibility is to require the caller to pass in a buffer; they ought to
> > already know how big it needs to be, based on the device type (and can always
> > just make it be NM_UTILS_HWADDR_LEN_MAX if not).
> 
> Looks ugly. What about just returning const address and letting the caller copy
> it if it needs to keep it?

Sure. I was thinking that your internal pointer wasn't guaranteed to stay good after you returned, but I guess it comes from priv->link_cache, so it's good at least until the next NMPlatform call or netlink message.

(In reply to comment #7)
> It seems like a pretty good match; the number of unique interface names NM will
> see during a given run is fairly small. (Eg, it's probably less than the total
> number of GObject types used by NM, each of which has an interned name.)

I was afraid it would be seen as sort of a memory leak on systems with long-running NetworkManager and virtual interfaces created on demand.

I think it will be better to return a const here and use rtnl_link_get instead of rtnl_link_i2name for that. You already confirmed the same idea for get_address.

> > It works for both bridges and bonds, at least with kernels I tested.
> 
> Hm... OK. I wonder why Thomas didn't do that originally then? Maybe just to
> avoid having separate nl3 and fallback codepaths I guess...

I think so. His bridging implementation was more of sketch than a full implementation.

> > Index should never be wrong, under normal circumstances. 
> 
> OK, but lots of methods just return NL_PLATFORM_ERROR_NOTFOUND in that case...

This is closely related to error handling. I guess I'll leave the error handling discussion for later as it will be necessary to go through the whole
code anyway.

> Sure. I was thinking that your internal pointer wasn't guaranteed to stay good
> after you returned, but I guess it comes from priv->link_cache, so it's good at
> least until the next NMPlatform call or netlink message.

Yes. We don't do threading with nm-platform. The returned value can be used as we don't make any changes to the particular link and we don't run the next main loop step. That probably needs some documentation.

But the number #1 case for nm_platform_link_index_to_name is for logging purposes. Using g_strdup for other cases shouldn't be a big deal. Is there an easy way to test NetworkManager for not reading deallocated memory, just in case? Valgrind maybe?

Ok, simplified the directory structure and fixed suppord for code coverage (see commit message).

Thinking about renaming:

link_index_to_name → link_get_name (index is the primary identifier)
link_name_to_index → link_get_index (name is the secondary identifier)

Known issues:

* error reporting
* code allignment (I asked on ML whether we really need to insist on code allignment instead of just indenting the stuff which makes much more sense to me)
* not full coverage (but pretty good, if you look carefully)

Another known issue:

* code coverage is not enabled by default (as dcbw requested)

is this punted to 0.9.10?

The platform-link targets 0.9.8 (unless we change the decision), porting of other code targets partially 0.9.8, partially 0.9.10.

*** This bug has been marked as a duplicate of bug 696434 ***