[Sugar-devel] [PATCH] Simple NetworkManager-0.9 port

[Daniel Drake]

Thanks for the review. I've made all changes except for the ones listed below:

On Mon, Oct 24, 2011 at 6:41 PM, Sascha Silbe <silbe at activitycentral.com> wrote:
> [extensions/cpsection/modemconfiguration/view.py]
> [ModemConfiguration.__init__()]
>> +        self._timeout_sid = 0
>
> How about _timeout_source_id? It took me some time to figure out what
> "sid" is supposed to mean. (Yes, I'm aware you only moved it.)

Yes. I'm not making changes to existing code where they aren't
necessary at this stage. I will follow up with further cleanup work
moving this to gobject introspection later. If you want to fix
existing stylistic items in the meantime I have no objection but I
won't be doing it myself.

>> -    def undo(self):
>> -        self._model.undo()
>
> What happened to the undo functionality? Since we still apply the
> changes after a timeout rather than after closing the dialog with the
> "apply" / "OK" button, we need this for the "cancel" button.

I couldn't figure out how this is supposed to work. It looks broken -
where is undo implemented? undo with an instant-apply configuration
system doesn't make much sense to me.

> [class WirelessDeviceView]
>>      def __deactivate_connection_cb(self, palette, data=None):
>> -        if self._mode == network.NM_802_11_MODE_INFRA:
>> -            connection = network.find_connection_by_ssid(self._name)
>> -            if connection:
>> -                connection.disable_autoconnect()
>> -
>>          network.disconnect_access_points([self._active_ap_op])
>
> Wouldn't this reintroduce SL#1802 [1]? I guess something like the
> following should do the trick:
>
>            settings = connection.get_settings()
>            settings['connection']['autoconnect'] = False
>            connection.update_settings(settings)
>
> The same applies to WirelessNetworkView.

Disabling autoconnect is not what you want any more - this would mean
that the network would not be connected to after rebooting, as this
setting persists.

This new code matches the way that nm-applet does it and seems to work OK.

> [class GsmDeviceView]
>>      def _update_state(self, state, old_state, reason):
>>          gsm_state = None
>>
>> -        if state is network.DEVICE_STATE_ACTIVATED:
>> +        if state is network.NM_DEVICE_STATE_ACTIVATED:
>
> Since you're already touching these lines, how about replacing the "is"
> with "=="? While the former does work in the specific case the constants
> are integers (and we might rely on that fact for the range comparisons
> as noted above), it's bad practice IMO.

As noted above I'm not yet going to make this kind of effort for
existing code that does not need to be modified, but I would not
object to others doing it.

> [src/jarabe/desktop/networkviews.py]
> [WirelessNetworkView.__init__()]
>> -        interface_props.Get(_NM_DEVICE_IFACE, 'State',
>> -                            reply_handler=self.__get_device_state_reply_cb,
>> -                            error_handler=self.__get_device_state_error_cb)
>> -        interface_props.Get(_NM_WIRELESS_IFACE, 'WirelessCapabilities',
> [...]
>>      def __get_active_ap_reply_cb(self, ap_path):
>>          self.__update_active_ap(ap_path)
>> +        interface_props = dbus.Interface(self._device, dbus.PROPERTIES_IFACE)
>> +        interface_props.Get(network.NM_DEVICE_IFACE, 'State',
>> +                            reply_handler=self.__get_device_state_reply_cb,
>> +                            error_handler=self.__get_device_state_error_cb)
>
> Is there a reason for the change of sequence?

Yes. Now that networks may be set up and connected to outside of
Sugar, the code must not assume that networks start off as
disconnected like it did before. So, in the current (and slightly
weird) design of the code, the active AP must be known before deciding
how to parse device state, since that information is used in the
decisions that are made. Without this, if NM connects to a network
during boot, it won't show as connected when Sugar later starts up.

> [WirelessNetworkView._connect()]
>> +        # Otherwise, create new connection and activate it
>> +        logging.debug("Creating new connection for %s", self._name)
>> +        settings = Settings()
>> +        settings.connection.id = str(self._name)
>
> We should check that this doesn't introduce even more problems with
> non-ASCII SSIDs. See SL#2023 [3], SL#2099 [4].
>
> Also, is there a reason we change the name from 'Auto <SSID>' to just
> '<SSID>'? Did NM / nm-applet do the same? (I don't mind the change, just
> wondering about the reason.)

Yes, the "Auto" thing is gone, and connecting to networks with
non-ascii characters works fine in my testing.

> [deactivate_active_channel()]
>> +        netmgr_props.Get(network.NM_IFACE, 'ActiveConnections', \
>>                  reply_handler=self.__get_active_connections_reply_cb,
>>                  error_handler=self.__get_active_connections_error_cb)
>
> No need for the backslash here, implied line continuation (open
> parenthesis) is enough.

Again, the code was already like this.

>> +    @dbus.service.method(NM_SECRET_AGENT_IFACE,
>>                           async_callbacks=('reply', 'error'),
>> +                         in_signature='a{sa{sv}}osasb',
>> +                         out_signature='a{sa{sv}}',
>> +                         sender_keyword='sender',
>> +                         byte_arrays=True)
>> +    def GetSecrets(self, settings, connection_path, setting_name, hints,
>> +                   request_new, reply, error, sender=None):
>> +        if setting_name != '802-11-wireless-security':
>> +            raise ValueError("Unsupported setting type %s" % setting_name)
>> +        if not sender:
>> +            raise Exception("Internal error: couldn't get sender")
>> +        uid = self._bus.get_unix_user(sender)
>> +        if uid != 0:
>> +            raise Exception("UID %d not authorized" % uid)
>> +
>> +        response = SecretsResponse(reply, error)
>> +        try:
>> +            self.secrets_request.send(self, settings=settings, response=response)
>> +        except Exception:
>> +            logging.exception('Error requesting the secrets via dialog')
>
> Shouldn't we return something?

Don't think so - why do you think this is needed?

> Also, what gets blocked while we're waiting for the dialog to get filled
> in and submitted? Would it make sense to make this method asynchronous?

This is already asynchronous, not sure what makes you think otherwise.

>> +    def is_sugar_internal_connection(self):
>> +        """Returns True if this connection is a 'special' Sugar connection,
>> +        i.e. one that has been created by Sugar internals and should not be
>> +        visible to the user or deleted by connection-clearing code."""
>> +        connection_id = self.get_id()
>> +        return connection_id == GSM_CONNECTION_ID \
>> +            or connection_id.startswith(ADHOC_CONNECTION_ID_PREFIX) \
>> +            or connection_id.startswith(MESH_CONNECTION_ID_PREFIX) \
>> +            or connection_id.startswith(XS_MESH_CONNECTION_ID_PREFIX)
>
> Using the 'in' operator makes it terser and IMO more readable:
>
>        return connection_id in (GSM_CONNECTION_ID, ADHOC_CONNECTION_ID_PREFIX,
>                                 MESH_CONNECTION_ID_PREFIX,
>                                 XS_MESH_CONNECTION_ID_PREFIX)
>
> We could even factor out the tuple into a global constant.

Your suggested replacement code does not match the behaviour of the
original, so I have left the original in place.

>> +        connections_o = settings.ListConnections()
>> +        for connection_o in connections_o:
>> +            self._monitor_connection(connection_o)
>
> Why the intermediary variable? It does not seem to provide any more
> information, nor would we otherwise need a line break.
> connection_path would be a better name for connection_o, BTW (the same
> applies to a few other places with *_o variables).

I've left them named as connection_o for now as a lot of the existing
code uses this name. But I have removed the temporary variable.

>> +    def clear(self):
>> +        """Remove all connections except Sugar-internal ones."""
>> +
>> +        # copy the list, to avoid problems with removing elements of a list
>> +        # while looping over it
>> +        connections = list(self._connections)
>> +        for connection in connections:
>> +            if connection.is_sugar_internal_connection():
>> +                continue
>> +            connection.delete()
>
> Should we really attempt to delete _all_ connections, including those
> created by other users and / or the administrator? What happens if we
> are disallowed from removing an individual connection (e.g. because it
> was set up by the administrator and we have no permission to remove it)?
> Will we end up with only some of the connections removed?

Connections which we do not have permission to access are not
presented to Sugar at all, so this already behaves OK.

> [_migrate_old_wifi_connections()]
>> +    os.unlink(config_path)
>
> We should rename the file rather than irrevocably destroying it.

I disagree - once the data has been migrated there is no need to store
a duplicate copy. The file will only be deleted if the migration
succeeds.

Daniel