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 741069 - [review] add versioning to linker script (version-script) for libnm [th/libnm-version-script-bgo741069]
[review] add versioning to linker script (version-script) for libnm [th/libnm...
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: NetworkManager maintainer(s)
NetworkManager maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2014-12-03 15:52 UTC by Thomas Haller
Modified: 2014-12-05 11:00 UTC
See Also:
GNOME target: ---
GNOME version: ---


Description Thomas Haller 2014-12-03 15:52:34 UTC 
Before we release 1.0, we should add versioning to the linker script for libnm.

Discussing with danw, it seems that this is not necessary for libnm-util/libnm-glib.
Comment 1 Thomas Haller 2014-12-03 15:53:19 UTC 
branch th/libnm-version-script-bgo741069
Comment 2 Dan Winship 2014-12-03 17:43:38 UTC 
> contrib/rpm: ignore libgsystem/ repository in build_clean.sh
> cli: g_strdup(NULL) returns NULL; simplify code for that

These don't seem to belong on this branch?

> build: adjust tools/check-exports.sh

I assume you've tested that your changes work if you move some symbols into another section?

> # Have to prefix with a tab and suffix with a ';' to match .ver file format

That comment is both backwards and in the wrong place now. (You now strip out the tab and ';' in get_syms_from_def() rather than adding them in get_syms().)

> libnm/build: add versioning to linker script

Semantically it seems like the "local: *;" should be in a separate section? (You can only have one "*", and logically, private symbols we add later shouldn't be  part of "libnm_1_0_0".)

And from reading the info page, it seems like "global:" is the default and doesn't need to be explicitly specified, so you could drop that.
Comment 3 Thomas Haller 2014-12-04 13:22:04 UTC 
(In reply to comment #2)
> > contrib/rpm: ignore libgsystem/ repository in build_clean.sh
> > cli: g_strdup(NULL) returns NULL; simplify code for that
> 
> These don't seem to belong on this branch?

no, they don't :)


> > build: adjust tools/check-exports.sh
> 
> I assume you've tested that your changes work if you move some symbols into
> another section?

Yes, it tested successfully.


> > # Have to prefix with a tab and suffix with a ';' to match .ver file format
> 
> That comment is both backwards and in the wrong place now. (You now strip out
> the tab and ';' in get_syms_from_def() rather than adding them in get_syms().)

fixed.


> > libnm/build: add versioning to linker script
> 
> Semantically it seems like the "local: *;" should be in a separate section?
> (You can only have one "*", and logically, private symbols we add later
> shouldn't be  part of "libnm_1_0_0".)

Systemd has (and keeps) the local part with the oldest section:
http://cgit.freedesktop.org/systemd/systemd/plain/src/libsystemd/libsystemd.sym.m4

but how about:

__libnm_local {
local:
»···*;
};

I pushed a fixup with that. OK?


> And from reading the info page, it seems like "global:" is the default and
> doesn't need to be explicitly specified, so you could drop that.

I'd prefer explicit. Ok?



rebased and pushed.
Comment 4 Dan Winship 2014-12-04 13:32:47 UTC 
(In reply to comment #3)
> __libnm_local {

> I pushed a fixup with that. OK?

Yes

> > And from reading the info page, it seems like "global:" is the default and
> > doesn't need to be explicitly specified, so you could drop that.
> 
> I'd prefer explicit. Ok?

I'm fine either way, but presumably if we say it in libnm_1_0_0, we also ought to say it into libnm_1_2_0 for consistency, etc, so there will eventually end up being lots of "global:"s. (Well, for small values of "lots" :)
Comment 5 Dan Williams 2014-12-05 01:03:24 UTC 
LGTM