<feed xmlns='http://www.w3.org/2005/Atom'>
<title>delta/NetworkManager.git/src/devices/nm-device-dummy.c, branch th/strsplit</title>
<subtitle>gitlab.freedesktop.org: NetworkManager/NetworkManager.git
</subtitle>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/NetworkManager.git/'/>
<entry>
<title>all: drop unnecessary includes of &lt;errno.h&gt; and &lt;string.h&gt;</title>
<updated>2019-02-12T07:50:28+00:00</updated>
<author>
<name>Thomas Haller</name>
<email>thaller@redhat.com</email>
</author>
<published>2019-01-31T12:44:13+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/NetworkManager.git/commit/?id=a3370af3a8a965e41950c5022182730d79614df2'/>
<id>a3370af3a8a965e41950c5022182730d79614df2</id>
<content type='text'>
"nm-macros-interal.h" already includes &lt;errno.h&gt; and &lt;string.h&gt;.
No need to include it everywhere else too.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
"nm-macros-interal.h" already includes &lt;errno.h&gt; and &lt;string.h&gt;.
No need to include it everywhere else too.
</pre>
</div>
</content>
</entry>
<entry>
<title>platform: merge NMPlatformError with nm-error</title>
<updated>2018-12-27T20:33:59+00:00</updated>
<author>
<name>Thomas Haller</name>
<email>thaller@redhat.com</email>
</author>
<published>2018-12-22T13:13:05+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/NetworkManager.git/commit/?id=d18f40320d1a5bb5f43a61f473d2cb6105da9b4e'/>
<id>d18f40320d1a5bb5f43a61f473d2cb6105da9b4e</id>
<content type='text'>
Platform had it's own scheme for reporting errors: NMPlatformError.
Before, NMPlatformError indicated success via zero, negative integer
values are numbers from &lt;errno.h&gt;, and positive integer values are
platform specific codes. This changes now according to nm-error:
success is still zero. Negative values indicate a failure, where the
numeric value is either from &lt;errno.h&gt; or one of our error codes.
The meaning of positive values depends on the functions. Most functions
can only report an error reason (negative) and success (zero). For such
functions, positive values should never be returned (but the caller
should anticipate them).
For some functions, positive values could mean additional information
(but still success). That depends.

This is also what systemd does, except that systemd only returns
(negative) integers from &lt;errno.h&gt;, while we merge our own error codes
into the range of &lt;errno.h&gt;.

The advantage is to get rid of one way how to signal errors. The other
advantage is, that these error codes are compatible with all other
nm-errno values. For example, previously negative values indicated error
codes from &lt;errno.h&gt;, but it did not entail error codes from netlink.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
Platform had it's own scheme for reporting errors: NMPlatformError.
Before, NMPlatformError indicated success via zero, negative integer
values are numbers from &lt;errno.h&gt;, and positive integer values are
platform specific codes. This changes now according to nm-error:
success is still zero. Negative values indicate a failure, where the
numeric value is either from &lt;errno.h&gt; or one of our error codes.
The meaning of positive values depends on the functions. Most functions
can only report an error reason (negative) and success (zero). For such
functions, positive values should never be returned (but the caller
should anticipate them).
For some functions, positive values could mean additional information
(but still success). That depends.

This is also what systemd does, except that systemd only returns
(negative) integers from &lt;errno.h&gt;, while we merge our own error codes
into the range of &lt;errno.h&gt;.

The advantage is to get rid of one way how to signal errors. The other
advantage is, that these error codes are compatible with all other
nm-errno values. For example, previously negative values indicated error
codes from &lt;errno.h&gt;, but it did not entail error codes from netlink.
</pre>
</div>
</content>
</entry>
<entry>
<title>core: give better error reason why device is incompatible with profile</title>
<updated>2018-07-24T07:39:09+00:00</updated>
<author>
<name>Thomas Haller</name>
<email>thaller@redhat.com</email>
</author>
<published>2018-06-27T15:00:55+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/NetworkManager.git/commit/?id=33a88ca566e87adce9102c3389568dfa0e200c85'/>
<id>33a88ca566e87adce9102c3389568dfa0e200c85</id>
<content type='text'>
Note the special error codes  NM_UTILS_ERROR_CONNECTION_AVAILABLE_*.
This will be used to determine, whether the profile is fundamentally
incompatible with the device, or whether just some other properties
mismatch. That information will be importand during a plain `nmcli
connection up`, where NetworkManager searches all devices for a device
to activate. If no device is found (and multiple errors happened),
we want to show the error that is most likely relevant for the user.

Also note, how NMDevice's check_connection_compatible() uses the new
class field "device_class-&gt;connection_type_check_compatible" to simplify
checks for compatible profiles.

The error reason is still unused.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
Note the special error codes  NM_UTILS_ERROR_CONNECTION_AVAILABLE_*.
This will be used to determine, whether the profile is fundamentally
incompatible with the device, or whether just some other properties
mismatch. That information will be importand during a plain `nmcli
connection up`, where NetworkManager searches all devices for a device
to activate. If no device is found (and multiple errors happened),
we want to show the error that is most likely relevant for the user.

Also note, how NMDevice's check_connection_compatible() uses the new
class field "device_class-&gt;connection_type_check_compatible" to simplify
checks for compatible profiles.

The error reason is still unused.
</pre>
</div>
</content>
</entry>
<entry>
<title>device: replace NM_DEVICE_CLASS_DECLARE_TYPES() macro by explicit initialization</title>
<updated>2018-07-24T07:39:09+00:00</updated>
<author>
<name>Thomas Haller</name>
<email>thaller@redhat.com</email>
</author>
<published>2018-07-10T07:26:42+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/NetworkManager.git/commit/?id=2ce416796772205e006c0a66188e0379465f0bfa'/>
<id>2ce416796772205e006c0a66188e0379465f0bfa</id>
<content type='text'>
It seems to me the NM_DEVICE_CLASS_DECLARE_TYPES() macro confuses more
than helping. Let's explicitly initialize the two fields, albeit with
another helper macro NM_DEVICE_DEFINE_LINK_TYPES() to get the list of
link-types right.

For consistency, also leave nop-lines like

  device_class-&gt;connection_type_supported = NULL;
  device_class-&gt;link_types = NM_DEVICE_DEFINE_LINK_TYPES ();

because all NMDevice class init methods should have this same
boiler plate code and to make it explicit that this is intended.
And there are only 3 occurences where this actually comes into play.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
It seems to me the NM_DEVICE_CLASS_DECLARE_TYPES() macro confuses more
than helping. Let's explicitly initialize the two fields, albeit with
another helper macro NM_DEVICE_DEFINE_LINK_TYPES() to get the list of
link-types right.

For consistency, also leave nop-lines like

  device_class-&gt;connection_type_supported = NULL;
  device_class-&gt;link_types = NM_DEVICE_DEFINE_LINK_TYPES ();

because all NMDevice class init methods should have this same
boiler plate code and to make it explicit that this is intended.
And there are only 3 occurences where this actually comes into play.
</pre>
</div>
</content>
</entry>
<entry>
<title>device: use NM_DEVICE_CLASS_DECLARE_TYPES() to set connection_type_supported of device class</title>
<updated>2018-07-24T07:39:09+00:00</updated>
<author>
<name>Thomas Haller</name>
<email>thaller@redhat.com</email>
</author>
<published>2018-07-10T06:27:04+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/NetworkManager.git/commit/?id=87a60c4596d097913977f14ceca2c76fda517092'/>
<id>87a60c4596d097913977f14ceca2c76fda517092</id>
<content type='text'>
the macro already does it just fine. Use it.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
the macro already does it just fine. Use it.
</pre>
</div>
</content>
</entry>
<entry>
<title>device: wrap NM_DEVICE_CLASS_DECLARE_TYPES() macro with do-while block</title>
<updated>2018-07-24T07:39:09+00:00</updated>
<author>
<name>Thomas Haller</name>
<email>thaller@redhat.com</email>
</author>
<published>2018-07-10T06:29:21+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/NetworkManager.git/commit/?id=0cbf2c8c2ae3311bae688449dea4cfa21f4293e6'/>
<id>0cbf2c8c2ae3311bae688449dea4cfa21f4293e6</id>
<content type='text'>
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
</pre>
</div>
</content>
</entry>
<entry>
<title>device/trivial: rename NMDeviceClass.connection_type to connection_type_supported</title>
<updated>2018-07-24T07:39:09+00:00</updated>
<author>
<name>Thomas Haller</name>
<email>thaller@redhat.com</email>
</author>
<published>2018-07-10T06:14:56+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/NetworkManager.git/commit/?id=b9ae79c273b069f48ea458855f5a3f1873b99e9c'/>
<id>b9ae79c273b069f48ea458855f5a3f1873b99e9c</id>
<content type='text'>
The term "connection_type" is overused. Give it a more distinct name.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
The term "connection_type" is overused. Give it a more distinct name.
</pre>
</div>
</content>
</entry>
<entry>
<title>all: remove consecutive empty lines</title>
<updated>2018-04-30T14:24:52+00:00</updated>
<author>
<name>Beniamino Galvani</name>
<email>bgalvani@redhat.com</email>
</author>
<published>2018-04-30T11:46:24+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/NetworkManager.git/commit/?id=1b5925ce881370a1aba347cea0afe61e6316e81a'/>
<id>1b5925ce881370a1aba347cea0afe61e6316e81a</id>
<content type='text'>
Normalize coding style by removing consecutive empty lines from C
sources and headers.

https://github.com/NetworkManager/NetworkManager/pull/108
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
Normalize coding style by removing consecutive empty lines from C
sources and headers.

https://github.com/NetworkManager/NetworkManager/pull/108
</pre>
</div>
</content>
</entry>
<entry>
<title>core: avoid clone of all-connections list for nm_utils_complete_generic()</title>
<updated>2018-03-20T14:08:18+00:00</updated>
<author>
<name>Thomas Haller</name>
<email>thaller@redhat.com</email>
</author>
<published>2018-03-14T07:57:42+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/NetworkManager.git/commit/?id=e17cd1d742c47122b159040ce23fbf684b21c486'/>
<id>e17cd1d742c47122b159040ce23fbf684b21c486</id>
<content type='text'>
NMSettings exposes a cached list of all connection. We don't need
to clone it. Note that this is not save against concurrent modification,
meaning, add/remove of connections in NMSettings will invalidate the
list.

However, it wasn't save against that previously either, because
altough we cloned the container (GSList), we didn't take an additional
reference to the elements.

This is purely a performance optimization, we don't need to clone the
list. Also, since the original list is of type "NMConnection *const*",
use that type insistently, instead of dependent API requiring GSList.

IMO, GSList is anyway not a very nice API for many use cases because
it requires an additional slice allocation for each element. It's
slower, and often less convenient to use.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
NMSettings exposes a cached list of all connection. We don't need
to clone it. Note that this is not save against concurrent modification,
meaning, add/remove of connections in NMSettings will invalidate the
list.

However, it wasn't save against that previously either, because
altough we cloned the container (GSList), we didn't take an additional
reference to the elements.

This is purely a performance optimization, we don't need to clone the
list. Also, since the original list is of type "NMConnection *const*",
use that type insistently, instead of dependent API requiring GSList.

IMO, GSList is anyway not a very nice API for many use cases because
it requires an additional slice allocation for each element. It's
slower, and often less convenient to use.
</pre>
</div>
</content>
</entry>
<entry>
<title>core/dbus: rework D-Bus implementation to use lower layer GDBusConnection API</title>
<updated>2018-03-12T17:37:08+00:00</updated>
<author>
<name>Thomas Haller</name>
<email>thaller@redhat.com</email>
</author>
<published>2018-02-26T12:51:52+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/NetworkManager.git/commit/?id=297d4985abcc7b571b8c090ee90622357fc60e16'/>
<id>297d4985abcc7b571b8c090ee90622357fc60e16</id>
<content type='text'>
Previously, we used the generated GDBusInterfaceSkeleton types and glued
them via the NMExportedObject base class to our NM types. We also used
GDBusObjectManagerServer.

Don't do that anymore. The resulting code was more complicated despite (or
because?) using generated classes. It was hard to understand, complex, had
ordering-issues, and had a runtime and memory overhead.

This patch refactors this entirely and uses the lower layer API GDBusConnection
directly. It replaces the generated code, GDBusInterfaceSkeleton, and
GDBusObjectManagerServer. All this is now done by NMDbusObject and NMDBusManager
and static descriptor instances of type GDBusInterfaceInfo.

This adds a net plus of more then 1300 lines of hand written code. I claim
that this implementation is easier to understand. Note that previously we
also required extensive and complex glue code to bind our objects to the
generated skeleton objects. Instead, now glue our objects directly to
GDBusConnection. The result is more immediate and gets rid of layers of
code in between.
Now that the D-Bus glue us more under our control, we can address issus and
bottlenecks better, instead of adding code to bend the generated skeletons
to our needs.

Note that the current implementation now only supports one D-Bus connection.
That was effectively the case already, although there were places (and still are)
where the code pretends it could also support connections from a private socket.
We dropped private socket support mainly because it was unused, untested and
buggy, but also because GDBusObjectManagerServer could not export the same
objects on multiple connections. Now, it would be rather straight forward to
fix that and re-introduce ObjectManager on each private connection. But this
commit doesn't do that yet, and the new code intentionally supports only one
D-Bus connection.
Also, the D-Bus startup was simplified. There is no retry, either nm_dbus_manager_start()
succeeds, or it detects the initrd case. In the initrd case, bus manager never tries to
connect to D-Bus. Since the initrd scenario is not yet used/tested, this is good enough
for the moment. It could be easily extended later, for example with polling whether the
system bus appears (like was done previously). Also, restart of D-Bus daemon isn't
supported either -- just like before.

Note how NMDBusManager now implements the ObjectManager D-Bus interface
directly.

Also, this fixes race issues in the server, by no longer delaying
PropertiesChanged signals. NMExportedObject would collect changed
properties and send the signal out in idle_emit_properties_changed()
on idle. This messes up the ordering of change events w.r.t. other
signals and events on the bus. Note that not only NMExportedObject
messed up the ordering. Also the generated code would hook into
notify() and process change events in and idle handle, exhibiting the
same ordering issue too.
No longer do that. PropertiesChanged signals will be sent right away
by hooking into dispatch_properties_changed(). This means, changing
a property in quick succession will no longer be combined and is
guaranteed to emit signals for each individual state. Quite possibly
we emit now more PropertiesChanged signals then before.
However, we are now able to group a set of changes by using standard
g_object_freeze_notify()/g_object_thaw_notify(). We probably should
make more use of that.

Also, now that our signals are all handled in the right order, we
might find places where we still emit them in the wrong order. But that
is then due to the order in which our GObjects emit signals, not due
to an ill behavior of the D-Bus glue. Possibly we need to identify
such ordering issues and fix them.

Numbers (for contrib/rpm --without debug on x86_64):

- the patch changes the code size of NetworkManager by
  - 2809360 bytes
  + 2537528 bytes (-9.7%)

- Runtime measurements are harder because there is a large variance
  during testing. In other words, the numbers are not reproducible.
  Currently, the implementation performs no caching of GVariants at all,
  but it would be rather simple to add it, if that turns out to be
  useful.
  Anyway, without strong claim, it seems that the new form tends to
  perform slightly better. That would be no surprise.

  $ time (for i in {1..1000}; do nmcli &gt;/dev/null || break; echo -n .;  done)
  - real    1m39.355s
  + real    1m37.432s

  $ time (for i in {1..2000}; do busctl call org.freedesktop.NetworkManager /org/freedesktop org.freedesktop.DBus.ObjectManager GetManagedObjects &gt; /dev/null || break; echo -n .; done)
  - real    0m26.843s
  + real    0m25.281s

- Regarding RSS size, just looking at the processes in similar
  conditions, doesn't give a large difference. On my system they
  consume about 19MB RSS. It seems that the new version has a
  slightly smaller RSS size.
  - 19356 RSS
  + 18660 RSS
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
Previously, we used the generated GDBusInterfaceSkeleton types and glued
them via the NMExportedObject base class to our NM types. We also used
GDBusObjectManagerServer.

Don't do that anymore. The resulting code was more complicated despite (or
because?) using generated classes. It was hard to understand, complex, had
ordering-issues, and had a runtime and memory overhead.

This patch refactors this entirely and uses the lower layer API GDBusConnection
directly. It replaces the generated code, GDBusInterfaceSkeleton, and
GDBusObjectManagerServer. All this is now done by NMDbusObject and NMDBusManager
and static descriptor instances of type GDBusInterfaceInfo.

This adds a net plus of more then 1300 lines of hand written code. I claim
that this implementation is easier to understand. Note that previously we
also required extensive and complex glue code to bind our objects to the
generated skeleton objects. Instead, now glue our objects directly to
GDBusConnection. The result is more immediate and gets rid of layers of
code in between.
Now that the D-Bus glue us more under our control, we can address issus and
bottlenecks better, instead of adding code to bend the generated skeletons
to our needs.

Note that the current implementation now only supports one D-Bus connection.
That was effectively the case already, although there were places (and still are)
where the code pretends it could also support connections from a private socket.
We dropped private socket support mainly because it was unused, untested and
buggy, but also because GDBusObjectManagerServer could not export the same
objects on multiple connections. Now, it would be rather straight forward to
fix that and re-introduce ObjectManager on each private connection. But this
commit doesn't do that yet, and the new code intentionally supports only one
D-Bus connection.
Also, the D-Bus startup was simplified. There is no retry, either nm_dbus_manager_start()
succeeds, or it detects the initrd case. In the initrd case, bus manager never tries to
connect to D-Bus. Since the initrd scenario is not yet used/tested, this is good enough
for the moment. It could be easily extended later, for example with polling whether the
system bus appears (like was done previously). Also, restart of D-Bus daemon isn't
supported either -- just like before.

Note how NMDBusManager now implements the ObjectManager D-Bus interface
directly.

Also, this fixes race issues in the server, by no longer delaying
PropertiesChanged signals. NMExportedObject would collect changed
properties and send the signal out in idle_emit_properties_changed()
on idle. This messes up the ordering of change events w.r.t. other
signals and events on the bus. Note that not only NMExportedObject
messed up the ordering. Also the generated code would hook into
notify() and process change events in and idle handle, exhibiting the
same ordering issue too.
No longer do that. PropertiesChanged signals will be sent right away
by hooking into dispatch_properties_changed(). This means, changing
a property in quick succession will no longer be combined and is
guaranteed to emit signals for each individual state. Quite possibly
we emit now more PropertiesChanged signals then before.
However, we are now able to group a set of changes by using standard
g_object_freeze_notify()/g_object_thaw_notify(). We probably should
make more use of that.

Also, now that our signals are all handled in the right order, we
might find places where we still emit them in the wrong order. But that
is then due to the order in which our GObjects emit signals, not due
to an ill behavior of the D-Bus glue. Possibly we need to identify
such ordering issues and fix them.

Numbers (for contrib/rpm --without debug on x86_64):

- the patch changes the code size of NetworkManager by
  - 2809360 bytes
  + 2537528 bytes (-9.7%)

- Runtime measurements are harder because there is a large variance
  during testing. In other words, the numbers are not reproducible.
  Currently, the implementation performs no caching of GVariants at all,
  but it would be rather simple to add it, if that turns out to be
  useful.
  Anyway, without strong claim, it seems that the new form tends to
  perform slightly better. That would be no surprise.

  $ time (for i in {1..1000}; do nmcli &gt;/dev/null || break; echo -n .;  done)
  - real    1m39.355s
  + real    1m37.432s

  $ time (for i in {1..2000}; do busctl call org.freedesktop.NetworkManager /org/freedesktop org.freedesktop.DBus.ObjectManager GetManagedObjects &gt; /dev/null || break; echo -n .; done)
  - real    0m26.843s
  + real    0m25.281s

- Regarding RSS size, just looking at the processes in similar
  conditions, doesn't give a large difference. On my system they
  consume about 19MB RSS. It seems that the new version has a
  slightly smaller RSS size.
  - 19356 RSS
  + 18660 RSS
</pre>
</div>
</content>
</entry>
</feed>
