Yeah, the points you list make sense.
I just wondering about joining the libraries. The main reason for the libraries split is that NM just needs libnm-util containing common code for NM and clients.
And libnm-glib is pure client library.
Of course, for clients one library is cleaner and more straightforward.

(In reply to comment #1)
> I just wondering about joining the libraries. The main reason for the libraries
> split is that NM just needs libnm-util containing common code for NM and
> clients.

Ah... missed that, although it's obvious now that you mention it since the daemon has classes with the same names as libnm-glib's classes...

So there are various things we could do. I think having a single library on the client side is nice though, so I'd suggest that we still merge everything together into one "libnm", but that all of the parts of that that the server needs also get built into a noinst library that the server links against.

(Or alternatively, rename things on either the client or server side so that there are no naming conflicts and we can just link the server to the whole library, even though it doesn't need the client-specific parts.)


Another thing to fix:

  - Rename NMSettingWired to NMSettingEthernet, and NMSettingWireless
    to NMSettingWifi (and likewise rename
    org.freedesktop.NetworkManager.Device.Wired and .Wireless,
    although we'd need to keep the old names there too for backward
    compatibility)

  - Make some of the NMSettings properties saner / less obviously a
    mapping of years of ifcfg-rh cruft. Eg, it's possible to create an
    NMSettingVlan with

        parent = eth0
        interface_name = wlan2.5
        id = 3

    which contradicts itself twice.

  - split s390 support into a subclass of NMDeviceEthernet

  - (partially?) merge GSM and CDMA stuff (or extract a superclass)

 - move MTU from the hardware-specific settings to NMSettingConnection

 - likewise with hwaddr and carrier; they aren't 100% generic, but we
   treat them as generic on the daemon side now anyway

 - Likewise cloned mac address? If so, we'd need to add a property saying
   whether or not you can really set it, since on some types you can't.

(In reply to comment #0)
> (This has been talked about in passing for a long time, but let's get it into
> bugzilla, even though it's probably not happening in the immediate future.)
> 
> libnm-util/libnm-glib have various issues and it would be nice if they didn't.
> 
> To avoid backward-compatibility problems, rather than changing the existing
> libnm-util/libnm-glib APIs, we could just leave them as they are, and start a
> new library for the new API. (There doesn't seem to be much point to the
> current split between two different libraries.) Then after a while, the old
> libs could go away. (Maybe they could be split off into a -compat package too.)

This looks pretty brave (maybe too brave?) now when we run for a large number of features and don't really have time for careful design. A through API redesign takes a lot of time, if one wants to have a good result.

And you don't even need to make up a new name if you want to break compatibility, you can also consider keeping two distinct versions of the library.

Also I think the most serious problems are in the libnm-util library that is used by NetworkManager itself and cannot be compatibly replaced. Also, I don't think it is a good idea to link NetworkManager against the client library as that will effectively block you from performing incompatible changes anyway.

>   - use GInetAddress rather than using guint32 for IPv4 and
>     struct in6_addr * for IPv6. Likewise use GInetAddressMask where
>     relevant.

Is there any real reason to use them instead of in_addr_t and struct in6_addr * in struct arrays and possibly string representation in the dbus API? Will it help anyone or just complicate things?

>   - The IP4 and IP6 types (NMSettingIP[46]Config, NMIP[46]Address,
>     NMIP[46]Route, NMIP[46]Config) should either be merged into combined
>     IP4-and-IP6 types, or else made subclasses of such a combined type.

The IP4 and IP6 configs in the client library and API are there mostly just to complicate things. It would be much better to query the device directly for each individual list of items, be it IPv4 addresses or IPv6 routes.

>     Combined with the previous, this should let us merge together lots
>     of code in the daemon (eg, nm_system_apply_ip4_config() and
>     nm_system_apply_ip6_config()) and thus get rid of a ton of duplicated
>     code.

That would reqire an API change and then there could be better solutions for the internal code.

>   - Routes and addresses should be kept more separate (especially
>     in IPv6, but also somewhat in IPv4,

I'm all for treating IPv4 and IPv6 the same.

When I look at your other suggestions, it sounds like you will break the dbus compatibility anyway and I'm not sure whether creating a compat library without a compat dbus interface is worth the effort.

The D-Bus interface could potentially be kept the same for most of this stuff, or at least could have compat API added fairly easily.  We could easily swap out libnm-util usage *internally* in the daemon for something else and keep the same D-Bus API.  That said, we need something like NMConnection internally anyway, and I'm not sure it makes a ton of sense to duplciate all that code once for the daemon and once for the client side.

What's a lot harder to break is the settings properties/values since that's what a lot of clients depend on, regardless of whether they use libnm-util (eg, KDE depends on their format too but obviously doesn't use libnm-util).  However, these can be worked around compatibly in some cases.

GSM vs. CDMA - we create a new "WWAN" setting combining any properties required for each type.  We leave the existing settings alone, so a connection can still be type=gsm or type=cdma.  But new connections could use type=wwan and that would handle new multi-mode devices too.  This new WWAN setting should include a *list* of APNs instead of just one, since devices these days multiple APNs for different functions.  It should remove the "preferred mode" stuff (eg 2G/3G preference) which should now be done out-of-band and not during connection.

s390 - we should create a new subclass of NMDeviceEthernet for s390 and a new s390 setting and convert the config plugins to the new type; and leave the old code and the NMSettingWired property the same.  Since these devices are actually *virtual* and can be created/removed on-demand, they are more like TUN/TAP instead of physical ethernet.

(In reply to comment #7)
> This looks pretty brave (maybe too brave?) now when we run for a large number
> of features and don't really have time for careful design.

This is intended for 0.10 at the earliest. And we already have a mostly-right API design now, this is just about fixing the known problems with it.

> And you don't even need to make up a new name if you want to break
> compatibility, you can also consider keeping two distinct versions of the
> library.

Maybe, but I don't like the current libnm-util/libnm-glib split anyway, so...

> Also I think the most serious problems are in the libnm-util library that is
> used by NetworkManager itself and cannot be compatibly replaced. Also, I don't
> think it is a good idea to link NetworkManager against the client library as
> that will effectively block you from performing incompatible changes anyway.

I'm not sure what you mean by this; code being used by NM itself is not a problem at all, since we can just change the code in src/ every time we change the code in libnm*/. The compatibility concerns come from *other* users of libnm*

> >   - use GInetAddress rather than using guint32 for IPv4 and
> >     struct in6_addr * for IPv6. Likewise use GInetAddressMask where
> >     relevant.
> 
> Is there any real reason to use them instead of in_addr_t and struct in6_addr *
> in struct arrays and possibly string representation in the dbus API? Will it
> help anyone or just complicate things?

dcbw suggested I should use strings in the D-Bus API for the new device types (eg, org.fd.NM.Device.Gre.Local and .Remote), and I think that is the best move for D-Bus. How they are exposed on the client side would depend on what people are primarily using them for... and of course, we can expose them as multiple types if that would be useful.

The important thing is to merge the handling of IPv4 and IPv6 together wherever it makes sense, rather than having lots of nearly-identical duplicate 4 and 6 functions and types.

> The IP4 and IP6 configs in the client library and API are there mostly just to
> complicate things. It would be much better to query the device directly for
> each individual list of items, be it IPv4 addresses or IPv6 routes.

It would have to be on the ActiveConnection, not the Device, since some VPN connections have no associated Device. (Assuming those types don't get merged together...) But that is probably fine, yes.

> When I look at your other suggestions, it sounds like you will break the dbus
> compatibility anyway and I'm not sure whether creating a compat library without
> a compat dbus interface is worth the effort.

The original idea was just to fix the C API, but yeah, some of these changes would require changing the D-Bus API too. But there are other things we want to do that would (ideally) change the D-Bus API too, like the aforementioned ActiveConnection/Device merge...

In some cases, it would not be a ton of effort to present both new and backward-compatible interfaces. Eg, as long as we keep the IP configs separate from the ActiveConnection within the daemon, then it's easy to keep exporting IP config D-Bus objects, but then we can just add properties to ActiveConnection as well to expose the same data directly from there.

(I'm not sure if GDBus would let us expose a single GObject as multiple (different) D-Bus objects...)


Anyway, this is just a list of all possible changes we might want to make. We might decide it makes sense to only do a subset of them.

Sounds good. The cleanup of Device versus ActiveConnection versus Connection would be really helpful, see also my comments to 703395.

I don't care much whether NetworkManager and the client library share code but if they do, I think that NM should not link to the library and that the common code should be in a separate *.so.

The question also is, what comes after NetworkManager 0.9.10. But that depends on whether we need substantial feature enhancement between 0.9.10 and 0.10.

*** Bug 702762 has been marked as a duplicate of this bug. ***

 - harmonize GObject property types with function return types (per
   bug 702762). Eg, GByteArray vs NM_TYPE_SSID vs DBUS_G_TYPE_UCHAR_ARRAY
   for SSIDs

looking at this along with bug 622927...

Notes on a few of the points above

>   - Use GDBus rather than dbus-glib.

that's 622927

>   - use GInetAddress rather than using guint32 for IPv4 and
>     struct in6_addr * for IPv6. Likewise use GInetAddressMask where
>     relevant.

Probably don't want to do this in most places, though as discussed in bug 682946 we should use strings for IP addresses in some places.

>   - The IP4 and IP6 types (NMSettingIP[46]Config, NMIP[46]Address,
>     NMIP[46]Route, NMIP[46]Config) should either be merged into combined
>     IP4-and-IP6 types, or else made subclasses of such a combined type.

Not sure about this...

>   - Routes and addresses should be kept more separate...

More 682946 stuff

>   - Make some of the NMSettings properties saner / less obviously a
>     mapping of years of ifcfg-rh cruft.

If we aren't breaking D-Bus compat, we're more limited here, but this is related to bug 696936

>  - move MTU (etc) from the hardware-specific settings to NMSettingConnection

Again, D-Bus compat issues, but the deprecation of the virtual interface-name properties in favor of NMSettingConnection:interface-name shows that we can do this.

One thing we could do here is to consolidate the NMSettingCdma and NMSettingGsm into an NMSettingWwan that would contain the settings of both.  Or at least, if we don't break compat here, add NMSettingWwan and deprecate the other two.

(In reply to comment #13)
> >   - use GInetAddress rather than using guint32 for IPv4 and
> >     struct in6_addr * for IPv6. Likewise use GInetAddressMask where
> >     relevant.
> 
> Probably don't want to do this in most places, though as discussed in bug
> 682946 we should use strings for IP addresses in some places.

Yeah, probably not... we store a lot more information about addresses than GInetAddress currently does.

> >   - The IP4 and IP6 types (NMSettingIP[46]Config, NMIP[46]Address,
> >     NMIP[46]Route, NMIP[46]Config) should either be merged into combined
> >     IP4-and-IP6 types, or else made subclasses of such a combined type.
> 
> Not sure about this...

Don't forget DHCP4 and DHCP6 config :)  These should really be combined into the same class on the client side, just use two instances, one for IP4 and one for IP6.  They both use strings right *now*, so we don't even need to change their API significantly.

NMIP4Address/NMIP6Address - could combine these into NMIPAddress and just use strings for the addresses, and add a 'family' property.  But it's really, really nice to be able to use guint32 for IP4 addresses, so ideally we have some mechanism to return the raw address as guint32 or struct in6_addr.  Maybe "guint32 nm_ip_address_get_ipv4_address ()" that asserts if the family != AF_INET?

I'd rather not use NMPlatformIPAddress here, because (a) that's in internal implementation detail, and (b) the platform contains *state* too instead of just configuration, and we don't want state in the Settings interface.  (however, we probably do want state in the NMIP4Config/NMIP6Config interface?)

NMIP4Route/NMIP6Route - same comments as for Address.

NMIP4Config/NMIP6Config - these could be combined client-side into the same class too, just different instances,

NMSettingIP4Config/NMSettingIP6Config - Depending on how you're redoing the Setting stuff, these would at least have to have different "type" names.  That could be done either with subclasses, or possibly more simply with a construct-time property that says which IP family the setting is used for?  But the more I think about it, the more I think we may want subclasses.  There are things that are IPv6 specific (privacy addresses, etc) that aren't relevant for IPv4 at all.

> >   - Routes and addresses should be kept more separate...
> 
> More 682946 stuff
> 
> >   - Make some of the NMSettings properties saner / less obviously a
> >     mapping of years of ifcfg-rh cruft.
> 
> If we aren't breaking D-Bus compat, we're more limited here, but this is
> related to bug 696936

For the VLAN example, it doesn't really contradict itself at all...  the interface name you noted there (wlan2.5) is completely legal for *any* interface, even a non-wlan one.  The user can set it to "adfasdfaf" if they want.  Having the "parent.id" is convention, but we certainly can't enforce that.

> >  - move MTU (etc) from the hardware-specific settings to NMSettingConnection
> 
> Again, D-Bus compat issues, but the deprecation of the virtual interface-name
> properties in favor of NMSettingConnection:interface-name shows that we can do
> this.

Yeah, we should probably do this.

OK, Thomas pointed out a while back that reviewing the entire rewrite all at once would really suck. So, I've pushed danw/libnm-initial to get things started (barely). It adds libnm, but its API isn't yet changed much from libnm-util/libnm-glib.

Notes:
  - Currently libnm-core.la is noinst, and it gets pulled in by libnm.la
    and by /sbin/NetworkManager. Meaning that the daemon has its own copy
    of those symbols which it is not sharing with any other processes. I'm
    not sure it's possible to fix this without having the split between
    the two libraries be public, which I don't want to do. (In particular,
    I'm pretty sure gobject-introspection won't let you have two libraries
    share a single namespace, so even if we only had a single pkgconfig
    file and a single include dir, we'd still need two girs/typelibs.)

  - On a related note, is there any reason to keep the "libnm" / "libnm-vpn"
    split? The VPN stuff doesn't add much code that would make it unwanted
    by libnm users, and all the VPN plugins link against libnm-glib now
    anyway, so having libnm-glib-vpn split out isn't helping them any either.

  - The branch includes a patch to stop building the documentation for
    libnm-util and libnm-glib, on the theory that building it is slow, and
    it will still be available on the web. But that patch could be removed.

  - I have a patch that moves libnm-util and libnm-glib into a deprecated/
    subdirectory, to reduce the number of "libnm*" directories... It's not
    on the branch because I decided against it, but maybe people would
    prefer that?

The libnm API isn't going to be stable for a while, so porting out-of-tree stuff to it might be painful (you'd only be able to compile today's network-manager-applet sources against today's NetworkManager sources, not yesterday's or tomorrow's). OTOH, having to add API to both the old and new libraries is also painful...

Oh, which brings me to one tricky part of all of this, which is that nm-settings.5, etc, are still generated from libnm-util, NOT libnm-core, on the theory that libnm-core's property types will be changing later to be less like the D-Bus representations (eg, NMSettingIP4Config:addresses will be a GPtrArray of NMIP4Address, not a D-Bus "aau"). Which would mean that we would have to add new settings properties to libnm-util, even if no one was going to use them... Or alternatively, maybe we should just commit the current generated nm-setting-docs.xml (to libnm-core), and treat that as the canonical source of setting property documentation from now on. (Whatever we do here obviously interacts with bug 683111...)

Regarding the "[TO BE SQUASHED]" comments, I would not squash those commits. Especially if they were created mostly executing some command, it is nice to follow how this commit came to be.


Also, the commit messages are useful. But could you rephrase them, so that I actually could copy&paste the commands and get the same result?

e.g. put a # hash before the comments:

      # Replace internal references to "libnm-util" and "libnm-glib" with "libnm"
      perl -pi -e 's/libnm-(util|glib)/libnm/;' libnm-core/*.[ch] libnm-core/tests/*.[ch] libnm/*.[ch] libnm/tests/*.[ch]

      # Fix includes of the enum-types files
      perl -pi -e 's/nm-utils-enum-types/nm-core-enum-types/;' libnm-core/*.[ch] libnm-core/tests/*.[ch] libnm/*.[ch] libnm/tests/*.[ch]
      perl -pi -e 's/nm-glib-enum-types/nm-enum-types/;' libnm/*.[ch] libnm/tests/*.[ch]

      # Remove unused files
      git rm -f libnm-core/nm-setting-template.[ch] libnm/libnm_glib.[ch] libnm/libnm-glib-test.c


E.g. commit c3ef31f0e878d450785ca8cf4a29bdd8c09c5c6a indicates, that it was partly done by a script, and partly manual modification. Could you split the commit in parts that are done by script? (with the above comments to reproduce).

Or maybe better: merge b30685871f5ee70c226f99d186bfc323e312059a and c3ef31f0e878d450785ca8cf4a29bdd8c09c5c6a in a first part that is purely automated (giving the script in the commit message). Then a second commit doing manual changes.


Of course this means that some intermediate commits are not compilable. But I'd be fine with that (especially since this in the middle of a major rework). Maybe the commit message could point that fact out.



> docs: remove libnm-util/libnm-glib docs

I would not remove the ability to build docs. Anybody building them, should be able to get docs too. Also, even if they are deprecated, I can imagine that we still add some functions that users need.

Useful might be a configure flag to suppress building the deprecated libraries entirely for those who want to speed up compilation. That could even be the default.




> libnm: fix up class struct reserved slots

+    /* Padding for future expansion */
+    gpointer padding[4];

could we name it as convention _padding or _slot_padding, or _reserved, or _reserved_slots. I would like to see a leading underscore. I like _reserved_slots the best.

Also, I think the comment is not necessary, we really know what it is there for.

Also, I would have them instead
     void (*padding[4])(void);
or use a typedef for function pointers -- or GCallback.
Not a real world problem though, because I would think that glib only works on systems where a function pointer is the same size as a data pointer.


Actually, I would do instead:


#define _RESERVED_SLOTS(number_free_slots) \
    GCallback _reserved_slots[number_free_slots]; \
    GCallback _get_vtable;

and use

   _RESERVED_SLOTS(3);

(note that one additional slot (_get_vtable) is there, so if we run out of slots, we don't use up the last one but instead hack our own vtable.
_RESERVED_SLOTS(0) should expand to "GCallback _get_vtable;" only. Some macro foo could handle that when we need it later.



> libnm: rename NetworkManager.h and NetworkManagerVPN.h

could you split this commit, one that renames the header and one that rename IFACE?


> libnm: add NetworkManager.h, disallow including individual headers

I see the advantage of this, but doesn't it slow down compilation by including more then most clients want?


+#ifndef NETWORKMANAGER_H

do we have a convention for the include-guard define? I usually prefer some underscores, or a common prefix.



Didn't do a careful review yet (will follow later). But this is going to be great!!

(In reply to comment #17)
> Also, the commit messages are useful. But could you rephrase them, so that I
> actually could copy&paste the commands and get the same result?
> 
> e.g. put a # hash before the comments:

Can't; git-commit will eat the line then :)

It's not that painful to cut+paste multiple times. And it only matters until this code gets committed, and I'm the only one who's going to be doing it.

> > docs: remove libnm-util/libnm-glib docs
> 
> I would not remove the ability to build docs. Anybody building them, should be
> able to get docs too. Also, even if they are deprecated, I can imagine that we
> still add some functions that users need.

I don't think we should be adding functions to libnm-util and libnm-glib after libnm is stable. If users need some new function, they can port to the new library.

However, there is still going to be an interim period where libnm is committed-but-not-yet-stable-and-recommended, so I guess we shouldn't drop the old docs right away. (If we are going to drop them at all.)

> +    /* Padding for future expansion */
> +    gpointer padding[4];
> 
> could we name it as convention...

I used the style being used by recent additions to glib and gtk.

> Also, I think the comment is not necessary, we really know what it is there
> for.

True

> > libnm: add NetworkManager.h, disallow including individual headers
> 
> I see the advantage of this, but doesn't it slow down compilation by including
> more then most clients want?

In theory, but not noticeably so in practice.

(The thing that really slows down compilation is lack of parallelizability; we need to stop building each /tests/ subdirectory separately and suck all the test programs into src/Makefile.am...)

> +#ifndef NETWORKMANAGER_H
> 
> do we have a convention for the include-guard define? I usually prefer some
> underscores, or a common prefix.

We do have a convention, and that's it :-}. I wouldn't be opposed to someone doing a tree-wide update to something with more underscores.

(In reply to comment #18)
> (In reply to comment #17)
> > Also, the commit messages are useful. But could you rephrase them, so that I
> > actually could copy&paste the commands and get the same result?
> > 
> > e.g. put a # hash before the comments:
> 
> Can't; git-commit will eat the line then :)
> 
> It's not that painful to cut+paste multiple times. And it only matters until
> this code gets committed, and I'm the only one who's going to be doing it.

I anyway think in this case the entire pasted scripts should be indented with 2 spaces. Then it doesn't matter. Other solutions: https://stackoverflow.com/questions/2788092/start-a-git-commit-message-with-a-hashmark

So while I didn't go over the code with a fine-toothed comb, I have no problem with any of the cleanups in the branch.

One big question though; since we're not changing namespace here, couldn't we run into linkage problems in the future, where some consumer of libnm links to something older that links to libnm-util/libnm-glib, with incompatible symbols?  I'm not sure there's a great way to prevent that except for namespacing stuff though...

(In reply to comment #20)
> One big question though; since we're not changing namespace here, couldn't we
> run into linkage problems in the future, where some consumer of libnm links to
> something older that links to libnm-util/libnm-glib, with incompatible symbols?
>  I'm not sure there's a great way to prevent that except for namespacing stuff
> though...

gtk ran into this with the 2.0 -> 3.0 transition, although they did have some way to detect it and abort. I'll figure out what that was.

But anyway, it's much rarer for an app to be linked against NetworkManager from two separate dependencies than it is for it to be linked against gtk from two separate dependencies, so I don't think it will be much of a problem in practice.

(In reply to comment #17)
> Regarding the "[TO BE SQUASHED]" comments, I would not squash those commits.

OK, I squashed the "fix up files" and "add build infrastructure" commits, but left the remainder unsquashed. So now there's one commit that copies in the files unchanged, and one that makes it build into a working library.

> Also, the commit messages are useful. But could you rephrase them, so that I
> actually could copy&paste the commands and get the same result?
> 
> e.g. put a # hash before the comments:

did this, but do we really care to keep them in what actually gets committed?

> E.g. commit c3ef31f0e878d450785ca8cf4a29bdd8c09c5c6a indicates, that it was
> partly done by a script, and partly manual modification.

Changed so that it's now all done by scripts

> > docs: remove libnm-util/libnm-glib docs

dropped this

> > libnm: fix up class struct reserved slots

This now uses exactly the format glib does:

    /*< private >*/
    gpointer padding[4];

> > libnm: rename NetworkManager.h and NetworkManagerVPN.h
> 
> could you split this commit, one that renames the header and one that rename
> IFACE?

done, the IFACE commit is now immediately after the "fix the capitalization" commit.

(In reply to comment #20)
> One big question though; since we're not changing namespace here, couldn't we
> run into linkage problems in the future

Added a new commit to the end, "libnm, libnm-utils: error out if mixed libnm/libnm-util symbols are detected", based on the code in gtk. (We need to add the check to both libnm and libnm-utils, since there's no guessing *which* nm_utils_init() you'll end up running...)

(In reply to comment #22)
> (In reply to comment #17)
> > Regarding the "[TO BE SQUASHED]" comments, I would not squash those commits.
> 
> OK, I squashed the "fix up files" and "add build infrastructure" commits, but
> left the remainder unsquashed. So now there's one commit that copies in the
> files unchanged, and one that makes it build into a working library.


> libnm: add libnm/libnm-core (part 1)

the commit message should read:

    mkdir -p libnm-core/tests
    cp libnm-util/*.[ch] libnm-util/nm-version.h.in libnm-core/
    rm -f libnm-core/nm-version.h libnm-core/nm-setting-template.[ch]
    cp libnm-util/tests/*.[ch] libnm-core/tests/
    mkdir -p libnm/tests
    cp libnm-glib/*.[ch] libnm/
    rm -f libnm/libnm_glib.[ch] libnm/libnm-glib-test.c
    cp libnm-glib/tests/*.[ch] libnm/tests/
    cp libnm-glib/tests/*.py libnm/tests/

> libnm: add libnm/libnm-core (part 2)

this commit consists of a script generated part and a manual intervention. Could you split this commit in two, the second adding the build infrastructure? Or, if you prefer, squash the script generated part into "(part 1)" -- but I wouldn't do that.


> > Also, the commit messages are useful. But could you rephrase them, so that I
> > actually could copy&paste the commands and get the same result?
> > 
> > e.g. put a # hash before the comments:
> 
> did this, but do we really care to keep them in what actually gets committed?

I would care.



>> libnm: remove all deprecated functions and properties

if you remove NM_SETTING_GSM_NETWORK_TYPE and related settings, how is NetworkManager providing a backward compatible DBUS interface for those settings? And how can NM support old config files containing these options?


>> libnm, core, cli, tui: fix the capitalization of various types

doesn't some of these changes also affect the exported DBUS interface?
For example, NMVpnConnectionState

create_pppd_cmd_line() << adds trailing white space


>> libnm: make use of GParamSpecFlags and GParamSpecEnum

How can NM provided a backward compatible DBUS interface with changing the property types? Map all integers types, enum and flags on the DBUS protocol level to a same "numbers" DBUS type?


> > > libnm: fix up class struct reserved slots
> 
> This now uses exactly the format glib does:
> 
>     /*< private >*/
>     gpointer padding[4];

not much excited. Why do you dislike _RESERVED_SLOTS(3); Does it mess with gtk-doc?



> (In reply to comment #20)
> > One big question though; since we're not changing namespace here, couldn't we
> > run into linkage problems in the future
> 
> Added a new commit to the end, "libnm, libnm-utils: error out if mixed
> libnm/libnm-util symbols are detected", based on the code in gtk. (We need to
> add the check to both libnm and libnm-utils, since there's no guessing *which*
> nm_utils_init() you'll end up running...)


> libnm, libnm-utils: error out if mixed libnm/libnm-util symbols are detected

in such a case nm_utils_init() kills the application. Since nm_utils_init() already has an error out parameter, why not signal the failure?

Of if you presume that users are not checking for success of nm_utils_init(), we could register it via G_DEFINE_CONSTRUCTOR, couldn't we and make the function library internal.

(In reply to comment #23)
> > libnm: add libnm/libnm-core (part 2)
> 
> this commit consists of a script generated part and a manual intervention.
> Could you split this commit in two, the second adding the build infrastructure?

I don't think there's any advantage to that; the point in splitting part 1 and part 2 is so that you can see how things have changed from libnm-util/libnm-glib. Splitting it into 3 commits rather than 2 does not make things any better in that regard, and just increases the number of effectively do-nothing commits in the tree.

> >> libnm: remove all deprecated functions and properties
> 
> if you remove NM_SETTING_GSM_NETWORK_TYPE and related settings, how is
> NetworkManager providing a backward compatible DBUS interface for those
> settings? And how can NM support old config files containing these options?

Note that those properties are not used any more (they were only used by the MM 0.6 code). So "backward compatible" just means "silently ignore them". But, the code doesn't do that (it prints a warning), so that needs to be fixed.

> >> libnm, core, cli, tui: fix the capitalization of various types
> 
> doesn't some of these changes also affect the exported DBUS interface?
> For example, NMVpnConnectionState

The D-Bus interface just has a "uint" there. It doesn't care about enum type names at all.

> >> libnm: make use of GParamSpecFlags and GParamSpecEnum
> 
> How can NM provided a backward compatible DBUS interface with changing the
> property types? Map all integers types, enum and flags on the DBUS protocol
> level to a same "numbers" DBUS type?

Yes. (Note to self; make sure that no properties end up getting switched between int and uint as a result of this commit.)

> > > > libnm: fix up class struct reserved slots
> > 
> > This now uses exactly the format glib does:

> not much excited. Why do you dislike _RESERVED_SLOTS(3); Does it mess with
> gtk-doc?

Being consistent with glib and gtk+ means we automatically avoid problems with gtk-doc and gobject-introspection in the future.

> > libnm, libnm-utils: error out if mixed libnm/libnm-util symbols are detected
> 
> in such a case nm_utils_init() kills the application. Since nm_utils_init()
> already has an error out parameter, why not signal the failure?

Because it's not a recoverable error. The binary is completely broken.

> we could register it via G_DEFINE_CONSTRUCTOR, couldn't we and make the
> function library internal.

We could do that too. I guess gtk didn't do it that way because there may be platforms without working constructors.

(In reply to comment #24)
> (In reply to comment #23)
> > > libnm: add libnm/libnm-core (part 2)
> > 
> > this commit consists of a script generated part and a manual intervention.
> > Could you split this commit in two, the second adding the build infrastructure?
> 
> I don't think there's any advantage to that; the point in splitting part 1 and
> part 2 is so that you can see how things have changed from
> libnm-util/libnm-glib. Splitting it into 3 commits rather than 2 does not make
> things any better in that regard, and just increases the number of effectively
> do-nothing commits in the tree.

if the commit is only generated by a script, I will verify that the script makes sense and that the commits diff is exactly produced by the script.

The review of manual changes, involves looking at what was actually changed in detail.

By splitting the commit (and providing the script) that becomes easier because the detailed review considers a smaller patch.


> > we could register it via G_DEFINE_CONSTRUCTOR, couldn't we and make the
> > function library internal.
> 
> We could do that too. I guess gtk didn't do it that way because there may be
> platforms without working constructors.

gobject/gtype.c has:

  #if defined (G_HAS_CONSTRUCTORS)
  [[snip]]
  G_DEFINE_CONSTRUCTOR(gobject_init_ctor)
  #else
  # error Your platform/compiler is missing constructor support
  #endif

so, I would say its save to assume that when having glib, we have support. I think we should do make nm_utils_init() private a separate commit.

(In reply to comment #23)
> > libnm: add libnm/libnm-core (part 1)
> 
> the commit message should read:
> 
>     mkdir -p libnm-core/tests

updated

> > libnm: add libnm/libnm-core (part 2)
...
> if the commit is only generated by a script, I will verify that the script
> makes sense and that the commits diff is exactly produced by the script.
> 
> The review of manual changes, involves looking at what was actually changed in
> detail.
> 
> By splitting the commit (and providing the script) that becomes easier because
> the detailed review considers a smaller patch.

updated the commit message to clarify that the scripted source code changes and the hand-written build system changes are orthogonal (with the former only applying to the .c and .h files, and the latter only applying to non-source-code files).

> >> libnm: remove all deprecated functions and properties
> >> libnm: make use of GParamSpecFlags and GParamSpecEnum

OK, these ended up having non-trivial side-effects, so in the interest of getting the initial libnm committed, I removed them from the branch for now. (They'll be back later...)

Both commits had non-side-effecty portions too though, so the branch now has:

  libnm: remove all deprecated functions and types

which removes deprecated functions but leaves deprecated properties in place

  libnm: add NetworkManager.h and NetworkManagerVPN.h to enum-types

which keeps that part of the GParamSpecFlags and GParamSpecEnum change, but drops the actual GParamSpecFlags and GParamSpecEnum part.

> > libnm, libnm-utils: error out if mixed libnm/libnm-util symbols are detected
...
> Of if you presume that users are not checking for success of nm_utils_init(),
> we could register it via G_DEFINE_CONSTRUCTOR, couldn't we and make the
> function library internal.

did that; this means that test-libnm-linking now crashes before it even reaches main(), so we have to use it as just a helper program and run the actual test from another program.

(re-pushed)

(In reply to comment #26)
> (In reply to comment #23)
> > > libnm: add libnm/libnm-core (part 1)
> > 
> > the commit message should read:
> > 
> >     mkdir -p libnm-core/tests
> 
> updated

- rm libnm/libnm_glib.[ch] libnm/libnm-glib-test.c libnm/nm-glib-enum-types.[ch]
+ rm -f libnm/libnm_glib.[ch] libnm/libnm-glib-test.c libnm/nm-glib-enum-types.[ch]


> > > libnm: add libnm/libnm-core (part 2)
> ...
> > if the commit is only generated by a script, I will verify that the script
> > makes sense and that the commits diff is exactly produced by the script.
> > 
> > The review of manual changes, involves looking at what was actually changed in
> > detail.
> > 
> > By splitting the commit (and providing the script) that becomes easier because
> > the detailed review considers a smaller patch.
> 
> updated the commit message to clarify that the scripted source code changes and
> the hand-written build system changes are orthogonal (with the former only
> applying to the .c and .h files, and the latter only applying to
> non-source-code files).

Ok.

Btw, you said you wanted to look into how NM actually *can* use libnm-core from libnm. Is that not possible then?




> > > libnm, libnm-utils: error out if mixed libnm/libnm-util symbols are detected
> ...
> > Of if you presume that users are not checking for success of nm_utils_init(),
> > we could register it via G_DEFINE_CONSTRUCTOR, couldn't we and make the
> > function library internal.
> 
> did that; this means that test-libnm-linking now crashes before it even reaches
> main(), so we have to use it as just a helper program and run the actual test
> from another program.

you don't want to make the whole nm_utils_init() G_DEFINE_CONSTRUCTOR? Why not, because it can fail and then the library user is supposed to handle that? But effectivley he can hardly handle it... just do not fail

(In reply to comment #27)
> - rm libnm/libnm_glib.[ch] libnm/libnm-glib-test.c
> libnm/nm-glib-enum-types.[ch]
> + rm -f libnm/libnm_glib.[ch] libnm/libnm-glib-test.c
> libnm/nm-glib-enum-types.[ch]

fixed

> Btw, you said you wanted to look into how NM actually *can* use libnm-core from
> libnm. Is that not possible then?

Well, no one seemed terribly concerned, so I haven't looked into it...

> you don't want to make the whole nm_utils_init() G_DEFINE_CONSTRUCTOR?

We want nm_client_new() to fail if nm_utils_init() fails, so there needs to be some non-constructor method for it to call... This is something we can improve later...


Re-pushed with the above commit message fix, plus these two commits added to the middle:

  e641e45 include: drop nm-settings-flags.h, move NMSecretAgentGetSecretsFlags
  f2cce2b libnm-core, etc: move NMSecretAgentCapabilities here

That had originally been part of bug 732867, but it caused libnm-glib API issues there, so I dropped it there, but then forgot to add it back to this branch.

> libnm-core, etc: move NMSecretAgentCapabilities here

a better commit message subject line?



> libnm: add register_properties() virtual method to NMObject

For NMClient and NMDevice, init_dbus_properties() also does other initialization, such as connecting signals.

I see, that in the next commit you remove the constructed() function, that
was used before.

Seems like:

+    g_signal_connect (object, "notify::" NM_CLIENT_WIRELESS_ENABLED,
+                      G_CALLBACK (wireless_enabled_cb), NULL);

should stay in constructed().


but
+         dbus_g_proxy_add_signal (priv->bus_proxy, "NameOwnerChanged",
+                                  G_TYPE_STRING, G_TYPE_STRING, G_TYPE_STRING,
+                                  G_TYPE_INVALID);
really should move. But then the name init_dbus_properties() is a bit off.

Anyway. Just a thought. I am fine with how it is.



Overall, looks good to me.

(In reply to comment #29)
> > libnm-core, etc: move NMSecretAgentCapabilities here
> 
> a better commit message subject line?

fixed

> For NMClient and NMDevice, init_dbus_properties() also does other
> initialization, such as connecting signals.

After some discussion on IRC, renamed init_dbus_properties() to init_dbus(), and made the distinction between that and construct() consistent across all objects.



Re-pushed with those changes plus some distcheck fixes and updates to contrib/fedora/rpm/NetworkManager.spec...

good to go?

> contrib: update NetworkManager.spec for libnm 

I think all the packages should now depend on libnm package (instead of glib)

e.g. nmtui as:
Requires: %{name}-glib%{?_isa} = %{epoch}:%{version}-%{release}

Also, could you move this commit earlier, I guess immediately after "all: port everything to libnm" (?)

(In reply to comment #31)
> I think all the packages should now depend on libnm package (instead of glib)

> Also, could you move this commit earlier, I guess immediately after "all: port
> everything to libnm" (?)

did those, fixed something else Thomas and I discussed on IRC yesterday, did some "distcheck" fixes, rebased, etc, etc, and pushed.

Further libnm improvements will happen on separate bugs. There will soon be a wiki page under https://wiki.gnome.org/Projects/NetworkManager laying out the full libnm plan...

(In reply to comment #32)
> Further libnm improvements will happen on separate bugs. There will soon be a
> wiki page under https://wiki.gnome.org/Projects/NetworkManager laying out the
> full libnm plan...

https://wiki.gnome.org/Projects/NetworkManager/libnm

So as an update, here is the stuff that had been talked about here or elsewhere that is NOT scheduled to happen for 1.0

  - use GInetAddress -- we're using strings in IP APIs instead

  - Rename NMSettingWired to NMSettingEthernet, and NMSettingWireless
    to NMSettingWifi -- I forget why I didn't do this now... it could
    still happen I guess...

  - Make some of the NMSettings properties saner / less obviously a
    mapping of years of ifcfg-rh cruft -- That idea was predicated on not
    preserving D-Bus compatibility.

  - split s390 support into a subclass of NMDeviceEthernet
  - (partially?) merge GSM and CDMA stuff (or extract a superclass)
    -- I didn't have a strong enough idea of what this should look like

  - move MTU from the hardware-specific settings to NMSettingConnection
  - likewise with hwaddr and carrier
  - Likewise cloned mac address?
    -- the existing of normalization makes it easy for us to deal with
       copying the properties from one setting to another now, so we could
       do this later and deprecate the old properties

  - Fix NMSettingBond to have individual properties rather than a single
    hash table. -- This can still happen later, we'll just be left with the
    deprecated options property in the API.

  - Drop nm_utils_init() -- maybe should still do that

  - NMSecretAgent API fixes -- maybe should still happen