712442 - Network API: Linux backend

Status: NEW

(Core :: Hardware Abstraction Layer (HAL), task, P3)

Description

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

This backend is using NetworkManager. I think we should probably extend it to also handle online/offline and get ride of the other NetworkManager service we have in the tree.

This is currently only working with NetworkManager 0.9. I will write another patch to make it work with NetworkManager 0.8. Depending on how much work it is, I will make it work with 0.7 or just make sure it doesn't run the instance with 0.7.

Comment 1

[Karl Tomlinson (:karlt)]

Comment on attachment 583299 [details] [diff] [review]
Patch v1

Given the history of incompatible changes to the NetworkManager D-Bus
Interface, I assume we need to check for the Interface version and only run
this code against that version, or other existing known compatible versions,
perhaps with a simple change for:

"A few new device states have been added, and all device states have been renumbered for flexibility."
http://projects.gnome.org/NetworkManager/developers/api/09/ref-migrating.html#id428973

Some lines over 80 columns can be wrapped.
Spaces around binary operators would make the code easier to read.

>+void moz_ptr_array_unref(GPtrArray* ptr) {
>+#if GLIB_MAJOR_VERSION >= 2 && GLIB_MINOR_VERSION >= 22
>+  g_ptr_array_unref(ptr);
>+#else
>+  g_ptr_array_free(ptr, true);
>+#endif
>+}

I don't think it is worth having this function.

g_ptr_array_free will compile and run with the intended purpose on all
versions.  The clients already assume they are the exclusive owners (because
they release the strings).  But a future user of the method may expect an
unref, given the function name, but get a free.

Use "TRUE" instead of "true" for gboolean parameters.

I wonder whether DBusUtils.h should be called DBusGLibUtils.h.

>+     * eDeviceState_Connected should have the same value as NM_DEVICE_STATE_ACTIVATED.
>+     * TODO: We should include NetworkManager headers to do that cleanly.
>+     *       That would require to have it installed in the build bots but
>+     *       doesn't make it mandatory to run the build.
>+     */
>+    enum State {
>+      eDeviceState_Disconnected,
>+      eDeviceState_Connected = 100

Mention NM_DEVICE_STATE_ACTIVATED is 100 only for NM D-Bus Interface
Specification version 0.9.

>+    char*       mPath;
>+    DBusGProxy* mProxy;
>+    Type        mType;
>+    State       mState;
>+    DBusGProxy* mSpecializedProxy;
>+    double      mSpeed;

dbus_g_proxy_get_path can be used instead of mPath, which would save some
memory management.

Please document the units of mSpeed.

Would using nsAutoRef for mProxy and mSpecializedProxy simplify things?

Consider an intelligent destructor for DeviceInfo and a copy constructor that
transfers ownership.

An object oriented design would have DeviceInfo listening for state and
property changes.  Allocating each DeviceInfo separately could simplify some
things, removing the need for copying, but, if you prefer to coalesce into one
allocation, the an array index could be passed through the signal handler user
data and GetInstance() used when needing to UpdateNetworkInfo().

When a DBusGProxy object will be destroyed, there is no need to explicitly
disconnect from the signal.  Given the restriction that
dbus_g_proxy_add_signal can only be called once on a proxy, it seems safe to
assume that nothing else is holding references to the DBusGProxys.

>+  enum Notify {
>+    eNotify,
>+    eDontNotify
>+  };

Avoiding boolean parameters is nice.
Swapping the order of eNotify and eDontNotify would mean that eNotify is true.

>+   * Clear mDevices and reset mDevicesCount. Disconnect to signal and free memory.

"Disconnect from signal"

>+   * The DBus proxy object to upower.

Needs updating for network manager.

>+  /**
>+   * Number of tracked devices.
>+   */
>+  guint mDevicesCount;
>+
>+  /**
>+   * Information of all tracked devices.
>+   */
>+  DeviceInfo* mDevices;

Would nsTArray be helpful here?

>+    sInstance = new NetworkManagerClient();

Can sInstance be deleted during shutdown?  Currently it is leaked.

>+  nsAutoRef<GPtrArray> devicesNM;

>+  if (!dbus_g_proxy_call(mNMProxy, "GetDevices", &error, G_TYPE_INVALID,
>+                         typeGPtrArray, &devicesNM, G_TYPE_INVALID)) {

The implicit reinterpret casts from nsAutoRef<T> to GPtrArray* and GHashTable*
to access the nsAutoRefs' private member variables is nasty.
The nsAutoRef is not really necessary for devicesNM, because there are no
early returns.  How about the approach proposed bug 698003 comment 1 for
hashTable?

There is no need to release in the error patch as the documentation for
dbus_g_proxy_end_call says:

  All D-Bus method calls can fail with a remote error. If this occurs, the
  error will be set and this function will return FALSE.

  Otherwise, the "out" parameters and return value of the method are stored in
  the provided varargs list.

>+    guint type = g_value_get_uint(static_cast<const GValue*>(g_hash_table_lookup(hashTable, "DeviceType")));

g_value_get_uint() does not necessarily check that the GValue is of type
G_TYPE_UINT, so skipping the G_VALUE_HOLDS_UINT() check is placing some faith
in the stability of the interface.  Given history, I'm not sure that is well
placed faith, but it looks like libnm-glib makes the same assumptions, and I
guess we have to only run this code against version 0.9 anyway, so I guess
this is expected.

I think it is still worth adding a null check for the GValue pointer, in case
the property is not there.

>+      // Note: Bitrate is in Kb/s according to the doc but nm-applet itself
>+      // shows it in Mb/s and does /1000 so there must be something wrong
>+      // somewhere and it seems better to be consistent with what nm-applet
>+      // shows.

I don't follow this comment.  Is that about the K instead of k?
The documentation says kilobits so they mean kb/s.
Or is there a 1024/1000 distinction somewhere?

>+      devices[i].mSpeed = g_value_get_uint(&value) / 1000.0;

Use mSpeed /= instead of a second g_value_get_uint call.

>+    dbus_g_proxy_add_signal(devices[i].mSpecializedProxy, "PropertiesChanged",
>+                            dbus_g_type_get_map("GHashTable", G_TYPE_STRING, G_TYPE_VALUE),
>+                            G_TYPE_INVALID);

dbus_g_type_get_map("GHashTable", G_TYPE_STRING, G_TYPE_VALUE) need only be
called once in InitializeDeviceList, though this call would be
appropriate if the DeviceInfo were managing the signals.

>+NetworkManagerClient::GetSpeedForDevice(const DeviceInfo& aDevice) const

This contains duplicate code, but may not be necessary anyway.  See below.

>+  // optimized sureley.

"surely"

>+    if (aNewState == DeviceInfo::eDeviceState_Connected) {
>+      devices[i].mState = DeviceInfo::eDeviceState_Connected;
>+    } else {
>+      devices[i].mState = DeviceInfo::eDeviceState_Disconnected;
>+      devices[i].mSpeed = 0;
>+    }

mSpeed is set to 0 on disconnect, but not explicitly reinstated on connect.
Apparently this is expecting a PropertyChanged on the specialized proxy.
Can we be sure to get that?  It seems safe to me to not set the speed to 0, as
we check mState in UpdateNetworkInfo anyway.

>+        if (!g_hash_table_lookup(aProperties, "Speed")) {
>+          // That means we are not connected yet but it seems that sometimes
>+          // we don't get "Speed" even when connected. In that case, reading
>+          // the value is working as expected.

I expect the non-existence of "Speed" in the hashtable should mean that the
speed didn't change, and there should be no need to get the speed again when
the property hasn't changed.  Is this fallout from setting speed to 0 above?

>+        newSpeed = g_value_get_uint(static_cast<const GValue*>(g_hash_table_lookup(aProperties, "Speed")));

Please call g_hash_table_lookup once only for each property.

Comment 2

[Mounir Lamouri (:mounir)]

Attachment: Inter diff

Comment 3

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

Comment 4

[Mounir Lamouri (:mounir)]

Sorry for the time it took me to answer to that review. I had different stuff to do.

(In reply to Karl Tomlinson (:karlt) from comment #1)
> Would using nsAutoRef for mProxy and mSpecializedProxy simplify things?

Not really. That would save us 2 lines in the dtor.

> >+    sInstance = new NetworkManagerClient();
> 
> Can sInstance be deleted during shutdown?  Currently it is leaked.

Isn't that how all singleton are done? The memory is released during shutdown.

> >+    if (aNewState == DeviceInfo::eDeviceState_Connected) {
> >+      devices[i].mState = DeviceInfo::eDeviceState_Connected;
> >+    } else {
> >+      devices[i].mState = DeviceInfo::eDeviceState_Disconnected;
> >+      devices[i].mSpeed = 0;
> >+    }
> 
> mSpeed is set to 0 on disconnect, but not explicitly reinstated on connect.
> Apparently this is expecting a PropertyChanged on the specialized proxy.
> Can we be sure to get that?  It seems safe to me to not set the speed to 0,
> as
> we check mState in UpdateNetworkInfo anyway.
> 
> >+        if (!g_hash_table_lookup(aProperties, "Speed")) {
> >+          // That means we are not connected yet but it seems that sometimes
> >+          // we don't get "Speed" even when connected. In that case, reading
> >+          // the value is working as expected.
> 
> I expect the non-existence of "Speed" in the hashtable should mean that the
> speed didn't change, and there should be no need to get the speed again when
> the property hasn't changed.  Is this fallout from setting speed to 0 above?

Unfortunately not. If I plug an ethernet cable, I don't get a change event unless I use that code. Even if I don't set mSpeed to 0.

All other comments have been applied in the recently attached patch.

Comment 5

[Mounir Lamouri (:mounir)]

Comment on attachment 605389 [details] [diff] [review]
Patch v2

Review of attachment 605389 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDOMClassInfo.cpp
@@ +4061,5 @@
>    DOM_CLASSINFO_MAP_END
>  
>    DOM_CLASSINFO_MAP_BEGIN(MozConnection, nsIDOMMozConnection)
>       DOM_CLASSINFO_MAP_ENTRY(nsIDOMMozConnection)
> +     DOM_CLASSINFO_MAP_ENTRY(nsIDOMEventTarget)

Forget that part of the patch. It will be handled by bug 735261.

Comment 6

[Karl Tomlinson (:karlt)]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #4)
> > Can sInstance be deleted during shutdown?  Currently it is leaked.
> 
> Isn't that how all singleton are done? The memory is released during
> shutdown.

Singletons are explicitly released during shutdown (at least in debug builds) before leak checks run before the program terminates.

Comment 7

[Mounir Lamouri (:mounir)]

(In reply to Karl Tomlinson (:karlt) from comment #6)
> (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #4)
> > > Can sInstance be deleted during shutdown?  Currently it is leaked.
> > 
> > Isn't that how all singleton are done? The memory is released during
> > shutdown.
> 
> Singletons are explicitly released during shutdown (at least in debug
> builds) before leak checks run before the program terminates.

Isn't that for XPCOM objects? AFAIK we don't check other leaks (I might be wrong though). It's not the first singleton I write in m-c like that.

Comment 8

[Karl Tomlinson (:karlt)]

The Lk results use trace-malloc, which detects all leaks from malloc and memalign.  I assume this also includes new.
(valgrind does something similar but I'm not sure of the state of our valgrind tests.)
See also NS_FREE_PERMANENT_DATA
http://hg.mozilla.org/mozilla-central/annotate/c71845b3b2a6/xpcom/base/nscore.h#l318

If we don't try to free our data, then leak detection tools become unhelpful.

Comment 9

[Karl Tomlinson (:karlt)]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #4)
> > >+    if (aNewState == DeviceInfo::eDeviceState_Connected) {
> > >+      devices[i].mState = DeviceInfo::eDeviceState_Connected;
> > >+    } else {
> > >+      devices[i].mState = DeviceInfo::eDeviceState_Disconnected;
> > >+      devices[i].mSpeed = 0;
> > >+    }
> > 
> > mSpeed is set to 0 on disconnect, but not explicitly reinstated on connect.
> > Apparently this is expecting a PropertyChanged on the specialized proxy.
> > Can we be sure to get that?  It seems safe to me to not set the speed to 0,
> > as
> > we check mState in UpdateNetworkInfo anyway.

I didn't find an answer to my question about the PropertyChanged event, but I
guess there will be a corresponding PropertyChanged event for the property State.

Why is mSpeed set to zero here?

> > 
> > >+        if (!g_hash_table_lookup(aProperties, "Speed")) {
> > >+          // That means we are not connected yet but it seems that sometimes
> > >+          // we don't get "Speed" even when connected. In that case, reading
> > >+          // the value is working as expected.
> > 
> > I expect the non-existence of "Speed" in the hashtable should mean that the
> > speed didn't change, and there should be no need to get the speed again when
> > the property hasn't changed.  Is this fallout from setting speed to 0 above?
> 
> Unfortunately not. If I plug an ethernet cable, I don't get a change event
> unless I use that code. Even if I don't set mSpeed to 0.

I see nm-device-ethernet.c has no calls to g_object_notify for
NM_DEVICE_ETHERNET_SPEED.

However, nm-device-wifi.c has a call for NM_DEVICE_WIFI_BITRATE, so there
shouldn't be a problem there.

Perhaps this is because ethernet device speeds are not expected to change.
I'm not sure.  On my system, this command returns 0 for my tg3 802-3-ethernet
device until the cable is first plugged.  Then is continues to report 100
after unplugging the cable.

> dbus-send --print-reply --system --dest=org.freedesktop.NetworkManager /org/freedesktop/NetworkManager/Devices/1 org.freedesktop.DBus.Properties.Get string:org.freedesktop.NetworkManager.Device.Wired string:Speed

Getting the speed after every property change (which BTW includes
org.freedesktop.NetworkManager.Device properties) seems a bit much.  How
about, for ethernet devices, getting the speed on StateChanged when new_state
becomes NM_DEVICE_STATE_ACTIVATED?

Comment 10

[Karl Tomlinson (:karlt)]

Perhaps PropertiesChanged could even handle changes to the State property and StateChanged would not be necessary, nor mProxy, nor the VOID:UINT,UINT,UINT marshall.

Comment 11

[Mounir Lamouri (:mounir)]

Attachment: Inter diff: make sure the memory is freed

I will attach a new patch including some other changes (which will all be in inter diff).

Comment 12

[Mounir Lamouri (:mounir)]

Attachment: Inter diff: improve speed handling

Comment 13

[Mounir Lamouri (:mounir)]

Attachment: Inter diff: merge StateChanged into PropertiesChanged

I didn't know that was possible to get changes from properties the interface was inheriting from...

Comment 14

[Mounir Lamouri (:mounir)]

Attachment: Patch v3

That patch should include all the requested changes made after I've attached the version 2. See inter diffs for more details.

Comment 15

[Mounir Lamouri (:mounir)]

(In reply to Karl Tomlinson (:karlt) from comment #9)
> Perhaps this is because ethernet device speeds are not expected to change.
> I'm not sure.  On my system, this command returns 0 for my tg3 802-3-ethernet
> device until the cable is first plugged.  Then is continues to report 100
> after unplugging the cable.
> 
> > dbus-send --print-reply --system --dest=org.freedesktop.NetworkManager /org/freedesktop/NetworkManager/Devices/1 org.freedesktop.DBus.Properties.Get string:org.freedesktop.NetworkManager.Device.Wired string:Speed

On my system, after unplugging, this is returning 0. Anyway, the new version of the patch is reducing the number of explicit read of Speed/Bitrate and should handle both cases.

Comment 16

[Karl Tomlinson (:karlt)]

Comment on attachment 605792 [details] [diff] [review]
Patch v3

>+  gpointer key, gvalue;
>+  if (!g_hash_table_lookup_extended(aHashTable, aKey, &key, &gvalue)) {
>+    return false;

Why use the the _extended function here?
Is there a reason why there should be a null GValue* in the hash table?

If there is a reason for using the _extended function, then pass NULL for
unused orig_key.

>+#if GLIB_MAJOR_VERSION >= 2 && GLIB_MINOR_VERSION >= 22
>+    g_ptr_array_unref(ptr);
>+#else
>+    g_ptr_array_free(ptr, TRUE);
>+#endif

Why bother with g_ptr_array_unref here?  (Comment 1)

>+NetworkManagerClient::DeviceInfo::DeviceInfo()
>+  : mDBusConnection(nsnull)
>+  , mType(eDeviceType_Unknown)
>+  , mState(eDeviceState_Disconnected)
>+  , mProxy(nsnull)
>+  , mSpeed(0)

mType and mState don't actually need to be initialized here.

>+  // There is no reason why getting the DBus connection object should fail
>+  // because the NetworkManagerClient already has a connection object if we
>+  // happen to be here.
>+  mDBusConnection = dbus_g_bus_get(DBUS_BUS_SYSTEM, nsnull);
>+
>+  // Make sure we do not exit the entire program if DBus connection get lost.
>+  dbus_connection_set_exit_on_disconnect(
>+      dbus_g_connection_get_connection(mDBusConnection), FALSE);

Given this is a preexisting connection, there is no need to call
dbus_connection_set_exit_on_disconnect here.

>+  nsAutoRef<DBusGProxy> proxy(dbus_g_proxy_new_for_name(
>+                                mDBusConnection,
>+                                NM_DBUS_SERVICE,
>+                                dbus_g_proxy_get_path(mProxy),
>+                                DBUS_INTERFACE_PROPERTIES));

How about dbus_g_proxy_new_from_proxy(mProxy, DBUS_INTERFACE_PROPERTIES, NULL)?
Then mDBusConnection may be unnecessary.

>+        // Sometimes, the hash table has a Speed or Bitrate field with the new
>+        // speed but tests shows that it's not always the case so it's better to
>+        // always explicitly request the speed instead of guessing when we
>+        // should do that.

Isn't this only an issue with the Wired Speed?  (Comment 9)

There should be notification for the wireless Bitrate, right?
(even if it may not be delivered at the same time as the change to State.)
The get_property implementation returns priv->rate and rate is only set when
notifying NM_DEVICE_WIFI_BITRATE.

>+    case DeviceInfo::eDeviceType_Ethernet: {
>+      guint speed;
>+      // If "Speed" is not in the hash table that means the speed didn't change.
>+      if (GetValueFromHashTable<guint, g_value_get_uint>(aProperties, "Speed", &speed)) {
>+        newSpeed = speed;

"Speed" is never in the hashtable, so the comment is not accurate here.  The
whole case block is unnecessary, but I guess it could be kept in case NM
implements this one day.

>+NetworkManagerClient::DeviceInfo::ListenForChanges()
>+{

>+  dbus_g_proxy_connect_signal(mProxy, "PropertiesChanged",
>+                              G_CALLBACK (PropertiesChanged), this, nsnull);
>+}

Please add some warnings that the copy constructor must not be used after this
method is called, or find a way to remove the copy constructor
altogether (and replace it with a private declaration).

>+  GPtrArray* devicesNM = nsnull;

I'm assuming devicesNM doesn't need to be initialized here.
(GError and GValue seem to be special.)

But it still needs to be released.

>+    nsAutoRef<DBusGProxy> deviceProxy(dbus_g_proxy_new_for_name(mDBusConnection,
>+                                                                  NM_DBUS_SERVICE,
>+                                                                  devicePath,
>+                                                                  NM_DBUS_INTERFACE_DEVICE));

deviceProxy is not really used.

>+    if (type != DeviceInfo::eDeviceType_Ethernet &&
>+        type != DeviceInfo::eDeviceType_Wifi) {
>+      continue;
>+    }

>+    const gchar* interface = nsnull;
>+
>+    switch (device.mType) {
>+      case DeviceInfo::eDeviceType_Ethernet:
>+        interface = NM_DBUS_INTERFACE_DEVICE_WIRED;
>+        break;
>+      case DeviceInfo::eDeviceType_Wifi:
>+        interface = NM_DBUS_INTERFACE_DEVICE_WIRELESS;
>+        break;
>+      default:
>+        MOZ_ASSERT(false);
>+    }
>+
>+    device.mProxy = dbus_g_proxy_new_for_name(mDBusConnection,
>+                                              NM_DBUS_SERVICE,
>+                                              dbus_g_proxy_get_path(deviceProxy),
>+                                              interface);

By moving the switch statement (and mProxy initialization) to the type check
above, they can merge into one test and |interface| need not be
null-initialized.

>+  const gchar* version = g_value_get_string(&value);
>+  return version[0] == '0' && version[1] == '.' && version[2] == '9';

Better to use a string comparison here.  This would match against 0.95 for
example.  There are also (possibly valid) assumptions here that the memory is
accessible even if the version string is short, but that may flag
uninitialized-read warnings in valgrind.

>+  if (!IsVersionSupported()) {
>+    // We do not support this version, we should stop here.
>+    dbus_connection_remove_filter(
>+        dbus_g_connection_get_connection(mDBusConnection),
>+        ConnectionSignalFilter, this);

Looks like there will be problems if ConnectionSignalFilter runs before this
point.

Would it be better to call dbus_connection_add_filter after checking the
version?  Or is it necessary to add checks that the connection is still up?

>+  // This should happen rarely enough to do something costly: we clear
>+  // everything we know about devices and re-initialize this. That could be
>+  // optimized surely.

Perhaps add a comment here that mDevices can't be adjusted while the proxies
have offsets into the array.