<feed xmlns='http://www.w3.org/2005/Atom'>
<title>delta/NetworkManager.git/src/settings/nm-settings.h, branch lr/python3</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>settings: pass in authentication subject to nm_settings_add_connection_dbus()</title>
<updated>2018-04-24T08:25:26+00:00</updated>
<author>
<name>Thomas Haller</name>
<email>thaller@redhat.com</email>
</author>
<published>2018-04-18T08:10:30+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/NetworkManager.git/commit/?id=8b5f64121143a6aec94dc8ba9c4059130822644c'/>
<id>8b5f64121143a6aec94dc8ba9c4059130822644c</id>
<content type='text'>
nm_settings_add_connection_dbus() has two callers. One of them is NMManager
during AddAndActivate. In this case, the NMActiveConnection already created
an auth-subject. Re-use it.

Note how creating an auth-subject involves reading procfs to determine
whether the process still exists. This is not about the additional
overhead of that, but about the race where the process could drop
of in the meantime. The calling process might be gone now, and we would
fail creating the auth-subject. There is no need for that, because we
already evaluated all information we need. Quite likely, in the case
of this race, PolicyKit will also determine that the process is gone
and fail authorization too. But that's PolicyKit's decision to make,
not nm_settings_add_connection_dbus()'s.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
nm_settings_add_connection_dbus() has two callers. One of them is NMManager
during AddAndActivate. In this case, the NMActiveConnection already created
an auth-subject. Re-use it.

Note how creating an auth-subject involves reading procfs to determine
whether the process still exists. This is not about the additional
overhead of that, but about the race where the process could drop
of in the meantime. The calling process might be gone now, and we would
fail creating the auth-subject. There is no need for that, because we
already evaluated all information we need. Quite likely, in the case
of this race, PolicyKit will also determine that the process is gone
and fail authorization too. But that's PolicyKit's decision to make,
not nm_settings_add_connection_dbus()'s.
</pre>
</div>
</content>
</entry>
<entry>
<title>settings: track connections via CList</title>
<updated>2018-04-13T07:09:46+00:00</updated>
<author>
<name>Thomas Haller</name>
<email>thaller@redhat.com</email>
</author>
<published>2018-03-29T12:48:42+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/NetworkManager.git/commit/?id=4127f1234fc4593cb80ad01435ac2b1743c8d5e2'/>
<id>4127f1234fc4593cb80ad01435ac2b1743c8d5e2</id>
<content type='text'>
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
</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>
<entry>
<title>settings: track visible state as regular connection flags</title>
<updated>2017-12-05T18:57:25+00:00</updated>
<author>
<name>Thomas Haller</name>
<email>thaller@redhat.com</email>
</author>
<published>2017-12-04T15:05:26+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/NetworkManager.git/commit/?id=0e1abe5ef365f5b271cb969c8df820214c56ec65'/>
<id>0e1abe5ef365f5b271cb969c8df820214c56ec65</id>
<content type='text'>
We already need to re-emit the notify::flags signal.
It's cumbersome to do this for boolean properties, so
re-use the flags to also track the visibility state.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
We already need to re-emit the notify::flags signal.
It's cumbersome to do this for boolean properties, so
re-use the flags to also track the visibility state.
</pre>
</div>
</content>
</entry>
<entry>
<title>settings: add "connection-flags-changed" signal to connection</title>
<updated>2017-12-05T18:57:25+00:00</updated>
<author>
<name>Thomas Haller</name>
<email>thaller@redhat.com</email>
</author>
<published>2017-12-04T14:54:03+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/NetworkManager.git/commit/?id=e704e827c4c3fc1f683cf727d806c027233672f0'/>
<id>e704e827c4c3fc1f683cf727d806c027233672f0</id>
<content type='text'>
We will need to subscribe to changes to "flags" from NMManager.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
We will need to subscribe to changes to "flags" from NMManager.
</pre>
</div>
</content>
</entry>
<entry>
<title>policy: use "agent-registered" signal directly from NMAgentManager instead of NMSettings</title>
<updated>2017-11-27T14:21:58+00:00</updated>
<author>
<name>Thomas Haller</name>
<email>thaller@redhat.com</email>
</author>
<published>2017-11-27T08:07:28+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/NetworkManager.git/commit/?id=46af70b508cdb19ce8fd6eafd01575d39124ed0e'/>
<id>46af70b508cdb19ce8fd6eafd01575d39124ed0e</id>
<content type='text'>
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
</pre>
</div>
</content>
</entry>
<entry>
<title>core: merge nm_settings_get_connections_sorted() with nm_settings_get_connections_clone()</title>
<updated>2017-11-27T13:04:11+00:00</updated>
<author>
<name>Thomas Haller</name>
<email>thaller@redhat.com</email>
</author>
<published>2017-11-22T11:46:58+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/NetworkManager.git/commit/?id=51531c953972215e0b15900463378fb8524b4232'/>
<id>51531c953972215e0b15900463378fb8524b4232</id>
<content type='text'>
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
</pre>
</div>
</content>
</entry>
<entry>
<title>hostname: split out hostname management from NMSettings</title>
<updated>2017-05-12T15:29:33+00:00</updated>
<author>
<name>Thomas Haller</name>
<email>thaller@redhat.com</email>
</author>
<published>2017-04-22T22:40:46+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/NetworkManager.git/commit/?id=5bfb7c3c89afe4da5b1ac2395391e9a986c722f0'/>
<id>5bfb7c3c89afe4da5b1ac2395391e9a986c722f0</id>
<content type='text'>
Hostname management is complicated. At least, how it is implemented currently.
For example, NMPolicy also sets the hostname (NMPolicy calls
nm_settings_set_transient_hostname() to have hostnamed set the hostname,
but then falls back to sethostname() in settings_set_hostname_cb()).
Also, NMManager tracks the hostname in NM_MANAGER_HOSTNAME too, and
NMPolicy listens to changes from there -- instead of changes from
NMSettings.

Eventually, NMHostnameManager should contain the hostname parts from NMSettings
and NMPolicy.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
Hostname management is complicated. At least, how it is implemented currently.
For example, NMPolicy also sets the hostname (NMPolicy calls
nm_settings_set_transient_hostname() to have hostnamed set the hostname,
but then falls back to sethostname() in settings_set_hostname_cb()).
Also, NMManager tracks the hostname in NM_MANAGER_HOSTNAME too, and
NMPolicy listens to changes from there -- instead of changes from
NMSettings.

Eventually, NMHostnameManager should contain the hostname parts from NMSettings
and NMPolicy.
</pre>
</div>
</content>
</entry>
<entry>
<title>policy: try to retrieve hostname from dbus first</title>
<updated>2017-03-24T14:18:08+00:00</updated>
<author>
<name>Francesco Giudici</name>
<email>fgiudici@redhat.com</email>
</author>
<published>2017-02-17T14:33:27+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/NetworkManager.git/commit/?id=bdd2e1c2aa0c8340743b307c3e78b38cda2a6934'/>
<id>bdd2e1c2aa0c8340743b307c3e78b38cda2a6934</id>
<content type='text'>
As we try to set the hostname through dbus, we should also try to
retrieve current hostname value from dbus first: otherwise we may end
retrieving the "old" hostname via gethostname while the dbus hostnamed
updated is pending.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
As we try to set the hostname through dbus, we should also try to
retrieve current hostname value from dbus first: otherwise we may end
retrieving the "old" hostname via gethostname while the dbus hostnamed
updated is pending.
</pre>
</div>
</content>
</entry>
<entry>
<title>include: use double-quotes to include our own headers</title>
<updated>2017-03-09T13:12:35+00:00</updated>
<author>
<name>Thomas Haller</name>
<email>thaller@redhat.com</email>
</author>
<published>2017-03-09T12:02:20+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/NetworkManager.git/commit/?id=831286df3001e6b76b7baeb10a7723841ab8b35e'/>
<id>831286df3001e6b76b7baeb10a7723841ab8b35e</id>
<content type='text'>
In practice, this should only matter when there are multiple
header files with the same name. That is something we try
to avoid already, by giving headers a distinct name.

When building NetworkManager itself, we clearly want to use
double-quotes for including our own headers.
But we also want to do that in our public headers. For example:

  ./a.c
    #include &lt;stdio.h&gt;
    #include &lt;nm-1.h&gt;
    void main() {
        printf ("INCLUDED %s/nm-2.h\n", SYMB);
    }

  ./1/nm-1.h
    #include &lt;nm-2.h&gt;

  ./1/nm-2.h
    #define SYMB "1"

  ./2/nm-2.h
    #define SYMB "2"

$ cc -I./2 -I./1 ./a.c
$ ./a.out
INCLUDED 2/nm-2.h

Exceptions to this are
  - headers in "shared/nm-utils" that include &lt;NetworkManager.h&gt;. These
    headers are copied into projects and hence used like headers owned by
    those projects.
  - examples/C
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
In practice, this should only matter when there are multiple
header files with the same name. That is something we try
to avoid already, by giving headers a distinct name.

When building NetworkManager itself, we clearly want to use
double-quotes for including our own headers.
But we also want to do that in our public headers. For example:

  ./a.c
    #include &lt;stdio.h&gt;
    #include &lt;nm-1.h&gt;
    void main() {
        printf ("INCLUDED %s/nm-2.h\n", SYMB);
    }

  ./1/nm-1.h
    #include &lt;nm-2.h&gt;

  ./1/nm-2.h
    #define SYMB "1"

  ./2/nm-2.h
    #define SYMB "2"

$ cc -I./2 -I./1 ./a.c
$ ./a.out
INCLUDED 2/nm-2.h

Exceptions to this are
  - headers in "shared/nm-utils" that include &lt;NetworkManager.h&gt;. These
    headers are copied into projects and hence used like headers owned by
    those projects.
  - examples/C
</pre>
</div>
</content>
</entry>
</feed>
