Excerpts from Daniel Drake's message of 2011-09-15 21:36:58 +0200: > Adapt sugar to NetworkManager-0.9 API changes. [...]
Thanks a lot for the patch!
[extensions/cpsection/modemconfiguration/model.py]
> +def get_settings(secrets_cb, secrets_err_cb):
> + connection = get_connection()
> + if not connection:
> + return '', '', ''
>
> + settings = connection.get_settings('gsm')
> + username = settings['username'] if 'username' in settings else ''
> + number = settings['number'] if 'number' in settings else ''
> + apn = settings['apn'] if 'apn' in settings else ''
Why not just settings.get('foo', '')?
>
> + connection.get_secrets('gsm', secrets_cb, secrets_err_cb)
> + return username, number, apn
This function returns values in two different ways (secrets_cb _and_
regular return values) at the same time. That's confusing, especially
since the callbacks don't get invoked at all if there's no connection.
The easiest way forward might be to pass everything to the callback;
e.g.:
class NoConnectionError(Exception):
"""No connection available to be used."""
def __init__(self):
Exception.__init__(self, 'No connection configured')
def get_settings(secrets_cb, secrets_err_cb):
connection = get_connection()
if not connection:
secrets_err_cb(NoConnectionError())
return
settings = connection.get_settings('gsm')
username = settings.get('username', '')
number = settings.get('number', '')
apn = settings.get('apn', '')
complete_cb = functools.partial(secrets_cb, username, number, apn)
connection.get_secrets('gsm', complete_cb, secrets_err_cb)
return username, number, apn
This is still not perfect as the timing is different: In the
no-connection case the error callback will be called before
get_settings() returns, whereas if we have a connection, there will be a
round-trip (usually even several) through the glib main loop before any
callback gets invoked. Maybe splitting it up into two separate functions
for secret and non-secret values is the better choice.
[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.)
[...]
> - self._puk_entry = PukEntry(model)
> - self._puk_entry.connect('notify::is-valid',
> - self.__notify_is_valid_cb)
> - self._group.add_widget(self._puk_entry.label)
> - self.pack_start(self._puk_entry, expand=False)
> - self._puk_entry.show()
> -
Is there a reason you dropped the PUK or is it just an oversight? If the
former, you should mention it in the commit message.
> + def _secrets_cb(self, secrets):
> + if secrets is None or not 'gsm' in secrets:
> + return
I prefer "foo not in bar" over "not foo in bar", but it might be just
personal taste; PEP-8 doesn't say anything about it.
> + gsm_secrets = secrets['gsm']
> + if 'password' in gsm_secrets:
> + self._populate_entry(self._password_entry,
> gsm_secrets['password'])
> + if 'pin' in gsm_secrets:
> + self._populate_entry(self._pin_entry, gsm_secrets['pin'])
Why do we make this conditional, rather than always filling with the
potentially empty value from the settings (using gsm_secrets.get() if
necessary)? Are there cases where we'd want an empty value in
gsm_secrets to not override an already-set value in the GUI?
> + def _secrets_err_cb(self, err):
> + logging.debug("No GSM secrets present, or error: %s", err)
In Sugar source, we prefer single quotes over double quotes for string
literals (unless the literal contains single quotes). An arbitrary
decision, but for consistency we should use the same style everywhere.
This applies to same other parts of the patch as well.
Is the "not present" case distinguishable from the error case? If so, we
should log with "error" level rather than "debug" in the error case.
> - 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.
[extensions/deviceicon/network.py]
[WirelessDeviceView._update_state()]
> - if state == network.DEVICE_STATE_PREPARE or \
> - state == network.DEVICE_STATE_CONFIG or \
> - state == network.DEVICE_STATE_NEED_AUTH or \
> - state == network.DEVICE_STATE_IP_CONFIG:
> + if state == network.NM_DEVICE_STATE_PREPARE or \
> + state == network.NM_DEVICE_STATE_CONFIG or \
> + state == network.NM_DEVICE_STATE_NEED_AUTH or \
> + state == network.NM_DEVICE_STATE_IP_CONFIG:
This list is incomplete (it lacks NM_DEVICE_STATE_IP_CHECK and
NM_DEVICE_STATE_SECONDARIES) and quite a lot of code for this simple
check. How about we make use of the fact that the numerical ordering was
designed to make this kind of check easy:
if network.NM_DEVICE_STATE_PREPARE <= state \
<= network.NM_DEVICE_STATE_IP_CONFIG:
It looks a bit weird due to having to wrap because of the long names for
the literals (without the "network." prefix it would fit on a single
line), so the long form might be better:
if (state >= network.NM_DEVICE_STATE_PREPARE) and \
(state <= network.NM_DEVICE_STATE_IP_CONFIG):
This applies to several other places in the patch.
[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.
[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.
[WEPKeyDialog.create_security()]
> + wsec = { 'wep-key0': key, 'auth-alg': auth_alg }
> + return { '802-11-wireless-security': wsec }
PEP-8 says not to add extra spaces inside braces. I.e.:
wsec = {'wep-key0': key, 'auth-alg': auth_alg}
return {'802-11-wireless-security': wsec}
Same for WPAKeyDialog.
[src/jarabe/desktop/meshbox.py]
[NetworkManagerObserver.__init__()]
> - self._bus = None
> + self._bus = dbus.SystemBus()
That would reintroduce SL#1403 [2]. AFAICT just dropping this particular
change and connecting to the bus in NetworkManagerObserver.listen() as
before should work fine.
[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?
[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.)
[src/jarabe/model/adhoc.py]
> + def _add_connection(self, channel):
> + ssid = 'Ad-hoc Network %d' % channel
As discussed before, a singleton tuple is preferred for consistency:
ssid = 'Ad-hoc Network %d' % (channel, )
This applies to same other parts of the patch as well.
[...]
> + return network.add_connection(settings)
Since add_connection returns None, the return is not necessary.
[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.
[src/jarabe/model/network.py]
> +_nm_manager = None
NetworkManager manager? How about _network_manager instead?
[...]
> +class SecretAgent(dbus.service.Object):
> + def __init__(self):
> + self._bus = dbus.SystemBus()
For the record: This is OK (i.e. would not reintroduce SL#1403 [2])
because SecretAgent only gets instantiated (indirectly) by
jarabe.desktop.meshbox.NetworkManagerObserver.listen(), after accessing
NetworkManager succeeded.
> + dbus.service.Object.__init__(self, self._bus, NM_SECRET_AGENT_PATH)
> self.secrets_request = dispatch.Signal()
> + proxy = dbus.SystemBus().get_object(NM_IFACE, NM_AGENT_MANAGER_PATH)
Why not use self._bus here?
> + @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?
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?
[...]
> def get_settings():
[...]
AFAICT this is only used inside jarabe.model.network, so we should use
the opportunity and make it private (i.e. rename to _get_settings()).
> + 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.
[...]
> +class Connections(object):
> + def __init__(self):
> + self._bus = dbus.SystemBus()
Since get_connections() gets called from various places, this would
reintroduce SL#1403. A similar solution as in
jarabe.desktop.meshbox.NetworkManagerObserver should do the trick.
Caveat: Settings (instantiated via get_settings()) also tries to access
the system bus.
[...]
> + 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).
[...]
> + def _monitor_connection(self, connection_o):
> + connection = Connection(self._bus, connection_o)
> + handler = connection.connect('removed', self._connection_removed_cb)
> + connection.set_data('removed-handler', handler)
> + self._connections.append(connection)
> +
> + def _new_connection_cb(self, connection_o):
> + self._monitor_connection(connection_o)
> +
> + def _connection_removed_cb(self, connection):
> + handler = connection.get_data('removed-handler')
> + connection.disconnect(handler)
> + connection.set_data('removed-handler', None)
> + self._connections.remove(connection)
Interesting trick (using get_data() / set_data() to store the handler
id). Is there any advantage over disconnecting by function? I.e.
something like this:
def _monitor_connection(self, connection_o):
connection = Connection(self._bus, connection_o)
connection.connect('removed', self._connection_removed_cb)
self._connections.append(connection)
def _connection_removed_cb(self, connection):
connection.disconnect_by_func(self._connection_removed_cb)
self._connections.remove(connection)
> + 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?
[...]
> +def find_connection_by_ssid(ssid):
> + # FIXME: this check should be more extensive.
> + # it should look at mode (infra/adhoc), band, security, and really
> + # anything that is stored in the settings.
> + connections = get_connections().get_list()
> + for connection in connections:
Like above, we could get rid of the intermediary variable. Same inside
find_connection_by_id().
[...]
> +def find_connection_by_id(search_id):
connection_id might be a better name. When I read search_id I'd expect
it to be the identity of a search (whatever that would be).
[...]
> +def add_connection(settings, reply_handler=_add_connection_reply_cb,
> + error_handler=_add_connection_error_cb):
> + return _nm_settings.AddConnection(settings.get_dict(),
> + reply_handler=reply_handler,
> + error_handler=error_handler)
This should use _get_settings() for consistency with
activate_connection_by_path() / add_and_activate_connection(). All of
these can probably only be invoked if NetworkManager has been found (and
the associated global variables initialised), but being careful doesn't hurt.
Since AddConnection() gets invoked with callbacks (turning it into an
asynchronous call), the return is not necessary and might mislead a
reader into assuming that a connection object is returned.
[_migrate_old_wifi_connections()]
> + os.unlink(config_path)
We should rename the file rather than irrevocably destroying it.
[_migrate_old_gsm_connection()]
> + create_gsm_connection(username, password, number, apn, pin)
> + # remove old connection
> + for setting in (GSM_USERNAME_PATH, GSM_PASSWORD_PATH,
> + GSM_NUMBER_PATH, GSM_APN_PATH, GSM_PIN_PATH,
> + GSM_PUK_PATH):
> + client.set_string(setting, '')
Not sure about removing the old settings, but we should at least change
the gconf descriptions (data/sugar.schemas.in) to indicate that these
are deprecated and changes won't be picked up.
[src/jarabe/model/olpcmesh.py]
> class OlpcMeshManager(object):
> + _CHANNEL_1 = 1
> + _CHANNEL_6 = 6
> + _CHANNEL_11 = 11
Why these constants? Do we expect the values to change?
There's only a place where they are use and IMO it doesn't benefit from
the in-place documentation that it's a channel number.
[OlpcMeshManager.ready()]
> + return network.add_connection(settings,
> +
> reply_handler=self._add_connection_reply_cb,
> +
> error_handler=self._add_connection_err_cb)
Since add_connection() returns None, the return is not necessary.
> + def _activate_connection(self, channel, xs_hosted):
> + connection = self._find_connection(channel, xs_hosted)
> + if connection:
> + connection.activate(self.mesh_device.object_path)
We should log a warning if no connection could be found.
This would have been easier to review if it had been split into several
patches; especially moving the constants to jarabe.model.network
contributed a lot of noise. Using --patience makes the patch more
readable as well (I've used it locally); --ignore-all-space was useful
for some parts (but shouldn't be used when sending the patch, of
course).
Sascha
[1] https://bugs.sugarlabs.org/ticket/1802
[2] https://bugs.sugarlabs.org/ticket/1403
[3] https://bugs.sugarlabs.org/ticket/2023
[4] https://bugs.sugarlabs.org/ticket/2099
--
http://sascha.silbe.org/
http://www.infra-silbe.de/
signature.asc
Description: PGP signature
_______________________________________________ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel
