After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 759261 - [review] platform cleanup [th/platform-api-cleanup-bgo759261]
[review] platform cleanup [th/platform-api-cleanup-bgo759261]
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: general
1.0.x
Other Linux
: Normal normal
: ---
Assigned To: NetworkManager maintainer(s)
NetworkManager maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2015-12-09 16:54 UTC by Thomas Haller
Modified: 2015-12-10 13:48 UTC
See Also:
GNOME target: ---
GNOME version: ---


Description Thomas Haller 2015-12-09 16:54:18 UTC 
After moving platform away from libnl-route-3 and adding support for new device types, do some refactoring and cleanup.


Refactoring is often problematic because it makes backporting harder. But at this point, platform deviated so far from nm-1-0 that we likely won't backport much anymore. The time is now.
Comment 1 Beniamino Galvani 2015-12-10 10:49:10 UTC 
> platform: return pointer to NMPlatformLink object for add functions

        /* Create any resources the device needs */
	if (NM_DEVICE_GET_CLASS (self)->create_and_realize) {
		if (!NM_DEVICE_GET_CLASS (self)->create_and_realize (self, connection, parent, &plink, error))
			return FALSE;
+               plink_copy = *plink;
+               plink = &plink_copy;
	}

-       NM_DEVICE_GET_CLASS (self)->setup_start (self, (plink.type != NM_LINK_TYPE_NONE) ? &plink : NULL);
-       nm_device_setup_finish (self, (plink.type != NM_LINK_TYPE_NONE) ? &plink : NULL);
+       NM_DEVICE_GET_CLASS (self)->setup_start (self, plink);
+       nm_device_setup_finish (self, plink)

I wonder if the copy is necessary... isn't the pointer supposed to be
valid through the whole function? But probably we can't be sure that
setup_start() and setup_finish() will not call any platform
operations which could invalidate the link. So LGTM, as the rest of
patches.
Comment 2 Thomas Haller 2015-12-10 11:54:00 UTC 
Exactly, the pointer is only guaranteed to be valid until the next nm_platform_*() operation.

It's not clear that setup_start()/setup_finish() doesn't do something that might invalidate the pointer, so we copy it.




Note that the lnk objects are guaranteed not to be modified by platform. Hence, with a NMPlatformLnk* object you could take an additional reference to the object.

Other platform objects (Link,IPxAddress,IPxRoute) are modified while they are cached. So, while we could do:

    nm_auto_nmpobj NMPObject *link_keep_alive = NULL;

    link_keep_alive = nmp_object_ref (NMP_OBJECT_UP_CAST (plink));

to keep plink alive, the content might still be modified by later platform operations.