[Dextrose] [PATCH v5][sucrose-0.94][RFC] Add capability to connect to WPA/WPA2-Enterprise Networks.
Ajay Garg
ajay at activitycentral.com
Tue Dec 20 01:18:18 EST 2011
Thanks Sascha for the review :)
Thanks a ton, for letting me know my subtle, hidden mistakes :) This
will help me write better production-quality code. Thanks a ton
again....
I need to digest the review-things; and make the required changes.
Will post the revised patch as soon as I am done.
Also, I agree, that peer review is the best thing to achieve robust,
high-quality, error-free code :)
Thanks again..
I will post the revised patch as soon as I am done.
Regards,
Ajay
On Tue, Dec 20, 2011 at 5:28 AM, David Farning
<dfarning at activitycentral.com> wrote:
> On Mon, Dec 19, 2011 at 8:14 AM, Sascha Silbe
> <sascha-ml-reply-to-2011-4 at silbe.org> wrote:
>> [Dropped sugar-devel from CC as the patch is based on a stable branch
>> rather than master]
>>
>>
>> Excerpts from Ajay Garg's message of 2011-12-15 10:55:13 +0100:
>>
>>> [PATCH v5][sucrose-0.94][RFC] Add capability to connect to WPA/WPA2-Enterprise Networks.
>>
>> The tags should all go inside the (same) brackets:
>> [PATCH RFC sugar sucrose-0.94 v5]
>>
>>> (Note that this is a consolidated patch, to be applied in full;
>>> and NOT OVER version-4, version-3, version-2, and version-1 patches).
>> [...]
>>
>> Please keep the changelog below the marker ("---") line.
>>
>> What I'm still missing is an explanation of what this patch does. What
>> exactly can a user do now that wasn't possible before (e.g. connecting
>> to WLANs using TTLS or PEAP for authentication - if possible include
>> links to explanations what these are)? What still isn't possible /
>> wasn't changed for some reason? How was it achieved technically? In what
>> way was the UI changed? What issues are there? What was tested
>> (successfully?), what wasn't? What alternative approaches were
>> considered and why was this one chosen (but only include information
>> relevant to Sugar (Labs), not internal business decisions of AC, OLPC
>> and / or whoever took part in the design and implementation of this
>> patch)?
>>
>> Some of this information is already spread out over the changelog and
>> needs to be summarised (in a coherent way) in the description; other
>> parts are missing entirely.
>>
>> The patch description is an important resource for reviewers (What is
>> the patch _supposed_ to do? Why was it done this way?), release note
>> authors (What changed since the last version and what does the user need
>> to know about it?) and developers hunting down bugs in the code you
>> touched (Why does the code work like this? How is it supposed to work?)
>>
>> Writing good patch descriptions is hard, but very important for
>> maintainability and long-term sustainability of a project.
>
> Sascha, Ajay.
>
> Thanks for slogging through this process reviewing and revising a
> major patch. While initially painful, the quality of the peer review
> is the single indicator of the success of a project.
>
> david
>
>>> ---
>>
>> This is the marker line. Changelogs should follow it immediately (before
>> the statistics part).
>>
>>
>> [src/jarabe/desktop/keydialog.py]
>>> @@ -20,9 +20,13 @@ from gettext import gettext as _
>>>
>>> import gtk
>>> import dbus
>>> +import os
>>> +import shutil
>>
>> Both os and shutil are part of the standard Python library, so they
>> should be imported in the previous block. See PEP 8 [1], section
>> "Imports".
>>
>>
>> [...]
>>> @@ -32,6 +36,11 @@ WEP_PASSPHRASE = 1
>>> WEP_HEX = 2
>>> WEP_ASCII = 3
>>>
>>> +SETTING_TYPE_STRING = 1
>>> +SETTING_TYPE_LIST = 2
>>> +SETTING_TYPE_CHOOSER = 3
>>
>> See comment below (for src/jarabe/desktop/networkviews.py).
>>
>>
>>> +
>>> +
>>>
>>
>> Please use exactly two blank lines to separate top-level definitions.
>> See PEP 8, section "Code layout", "Blank Lines". You can use the tool
>> pep8 to check for [2] this kind of mistake; your patch should not
>> trigger any output from the tool.
>>
>> This may seem a bit picky (and to some extent it is), but consistent
>> style is important for readability.
>>
>>
>>> +class NetworkParameters(gtk.HBox):
>>> + def __init__(self, auth_param):
>>
>> What is this class for? Please add a docstring and consider choosing a
>> more expressive name.
>>
>>
>> [...]
>>> + self._key = auth_param._key_name
>>
>> Please don't access private parts ("subclass API" in PEP 8 terms) of
>> "third-party" classes (i.e. those without an inheritance relationship to
>> your class). See PEP 8, section "Naming Conventions", "Prescriptive:
>> Naming Conventions", "Designing for inheritance". pylint [2] would have
>> complained about this (W0212).
>>
>> The purpose of the distinction between "public" and "private" (or
>> "protected" in some languages) is to allow changing the implementation
>> (the "private" parts) without breaking API for the callers (the "public"
>> part).
>>
>> pylint outputs some false positives for the Sugar source code alongside
>> some actual mistakes that need cleaning up, making it a bit harder to
>> see what messages your patch introduced. It's nevertheless a very useful
>> tool and you should run it directly prior to submitting your patch. If
>> you trigger additional pylint messages and don't think they should apply
>> to your code, please mention this in the patch comment (i.e. after the
>> marker line).
>>
>>
>> [...]
>>> + self._label = gtk.Label(_(auth_param._key_label))
>>
>> This isn't going to work as-is. _() not only looks up a translation, but
>> also marks a string in the source as needing translation, so it needs to
>> be a literal string rather than a variable or attribute.
>>
>> There are two ways to solve this (or rather three ways, as it turns
>> out later on in this review):
>>
>> 1. Use subclassing and pass the translated label to the superclass.
>>
>> 2. Pull a trick like sugar.util does for translating timestamps.
>>
>>
>> In this particular case, subclassing would make more sense: you can
>> introduce intermediary classes for the various parameter types
>> (single-valued, multi-valued and file) and get rid of the _is_entry(),
>> _is_liststore() and _is_chooser() methods.
>>
>> [...]
>>> + self._label.show()
>>
>> Unless you're going to hide some of the widgets, please use show_all()
>> on the container rather than littering the code with show() for the
>> individual widgets. GTK already is much too verbose, making it hard to
>> get a high-level view on the code.
>>
>>
>> [...]
>>> + elif self._is_liststore():
>>> + self._option_store = gtk.ListStore(str, str)
>>> + for option in auth_param._options:
>>> + self._option_store.append(option)
>>
>> This might be correct, but please take it as an opportunity to stop,
>> lean back and think about your interfaces. Where do the values come
>> from, where do they do? What encoding are the strings in? Are they / do
>> they need to be escaped in any way?
>>
>> GTK uses (read: both expects and gives you) UTF-8 and has a few
>> interfaces that scan the string for control sequences, requiring
>> escaping of strings we get from "the outside". IIRC NetworkManager uses
>> UTF-8 for most interfaces, but there are some that are binary data (e.g.
>> SSID). The NetworkManager D-Bus API docs should tell you more.
>>
>> We've had [3] resp. still have [4] issues with escaping and encoding in
>> the NetworkManager client code (which your patch touches), so we need to
>> make sure we don't introduce additional issues.
>>
>>
>>> + self._option_combo = gtk.ComboBox(self._option_store)
>>> + cell = gtk.CellRendererText()
>>> + self._option_combo.pack_start(cell, True)
>>> + self._option_combo.add_attribute(cell, 'text', 0)
>>> + self._option_combo.set_active(0)
>>> + self._option_combo.connect('changed',
>>> + self._option_combo_changed_cb)
>>> + self.pack_start(self._option_combo)
>>
>> Without taking a closer look, I guess this could be simplified. Check
>> out the "demo/menu.py" example from PyGTK
>> (/usr/share/doc/python-gtk2-doc/examples/demo/menu.py on Debian if you
>> have python-gtk2-doc installed; Fedora probably ships it in a similar
>> package):
>>
>> combo_box = gtk.combo_box_new_text()
>> combo_box.set_wrap_width(2)
>> for i in range(50):
>> combo_box.append_text('item - %d' % i)
>> combo_box.set_active(0)
>> vbox2.pack_start(combo_box)
>>
>>
>> No CellRenderer instantiation, no attribute declaration, just
>> gtk.combo_box_new_text() as constructor.
>>
>>
>> [...]
>>> + self._chooser_button = gtk.Button(_('Choose..'))
>>
>> This should probably tell the user exactly what to choose (and have
>> three dots rather than two).
>>
>>
>> [...]
>>> + def _is_entry(self):
>>> + return ( not self._is_chooser() ) and \
>>> + ( len(self._auth_param._options) == 0 )
>>
>> Please remove extra whitespace immediately inside parentheses. See
>> PEP 8, section "Whitespace in Expressions and Statements", "Pet Peeves".
>>
>> You don't need explicit line continuation (the lone backslash at the end
>> of the line) as there's still a parenthesis open, triggering implicit
>> line continuation. See also PEP 8, section "Code lay-out", "Maximum Line
>> Length".
>>
>> Testing (non-)emptiness should use the fact that empty sequences are
>> false, rather than invoking len(). See PEP 8, section "Programming
>> Recommendations". Besides making the code easier to read (less noise),
>> it can be faster in some cases (as the exact count doesn't need to be
>> calculated; this only applies to special objects and not to the built-in
>> lists, though).
>>
>>
>> [...]
>>> + def _object_chooser_cb(self, chooser_button):
>>> + self._want_document = True
>>> + self._show_picker_cb()
>>> +
>>> + def _show_picker_cb(self):
>>> + if not self._want_document:
>>> + return
>>
>> What's the purpose of _want_document?
>>
>>> + self._chooser = ObjectChooser()
>>> + self._chooser._set_callback(self.__process_selected_journal_object)
>>
>> Having to access the private API of
>> jarabe.journal.objectchooser.ObjectChooser suggests you should use
>> sugar.graphics.objectchooser.ObjectChooser (which is public API) rather
>> than jarabe.journal.objectchooser.ObjectChooser (which is an
>> implementation detail of the Journal).
>>
>>
>>> + def __process_selected_journal_object(self, object_id):
>>> + jobject = self._chooser.get_selected_object()
>>> + if jobject and jobject.file_path:
>> [a lot of code follows; only self._chooser.destroy() is called if the
>> above test fails]
>>
>> Please use "guardian style" instead:
>>
>> if not jobject or not jobject.file_path:
>> self._chooser.destroy()
>> return
>>
>> This makes it clear that in the "empty" case nothing else happens
>> (immediate return) and gets rid of one level of indentation, packing
>> more source code on the screen and helping to get an overview of the
>> code.
>>
>>
>>> + file_basename = \
>>> + os.path.basename(jobject._metadata._properties['title'])
>>
>> If you need continuations because of accessing nested objects, using a
>> temporary variable can increase readability without any expense in
>> terseness (as you'd need two lines anyway):
>>
>> object_title = jobject._metadata._properties['title']
>> file_basename = os.path.basename(object_title)
>>
>> (OK, in this case it didn't really help a lot ;) )
>>
>> There are two issues that worry me more, though:
>>
>> 1. Accessing private data of sugar.datastore.datastore.DSObject (which
>> isn't even in the same module as your code). What does the private
>> API (_metadata) give you that the public API (metadata) doesn't
>> offer?
>>
>> 2. Using random user data for file names and widget labels after partial
>> sanitation. The data store API supports storing NUL bytes, but POSIX
>> disallows them in file names and GTK works with C-style (i.e.
>> NUL-delimited) strings. Can this cause your code to misbehave in an
>> exploitable way after the user was tricked into choosing a specially
>> crafted Journal entry?
>>
>> Why do we need to turn the title into a file name in the first place?
>> It's not a unique identifier (only the object_id, 'uid' for the
>> current version of sugar-datastore, is unique) and NetworkManager
>> shouldn't care about the file name (if it does, we should take care
>> of that ourselves rather than forcing the user to handle it) - if it
>> even accesses the file, rather than us passing the content down via
>> D-Bus (haven't checked that part of your code yet).
>>
>>
>> [...]
>>> + # The chooser entries are chosen from the journal.
>>> + # Now, the journal entries misbehave, and the entries keep
>>> + # changing their names. Thus, in order to have the same
>>> + # entry available at later times, we need to store the
>>> + # selected entries at a 'fixed-name' place.
>>
>> What exactly do you consider misbehaviour and why?
>>
>>
>>> + # DISCOVERY-CUM-REQUIREMENT:
>>> + # -------------------------
>>
>> What is this supposed to mean? It doesn't make any kind of sense to me
>> (except for "REQUIREMENT").
>>
>>
>>> + profile_path = env.get_profile_path()
>>> + self._entry = os.path.join(profile_path, 'nm',
>>> + file_basename)
>>> +
>>> + # Remove (older) file, if it exists.
>>> + if os.path.exists(self._entry):
>>> + os.remove(self._entry)
>>> +
>>> + # Copy the file.
>>> + shutil.copy2(jobject.file_path, self._entry)
>>
>> This looks like it would "leak" files when the user chooses entries with
>> different titles. Using a constant name (see above) would fix this.
>>
>>
>>> + def _option_combo_changed_cb(self, widget):
>>> + it = self._option_combo.get_active_iter()
>>
>> Please don't abbreviate where it's not necessary and especially not when
>> abbreviating makes it hard to understand what the intention is. In this
>> case, "it" could mean "the object" (raising the question "what object");
>> "iterator" only comes to mind after taking a closer look at the code in
>> question.
>>
>>
>> [...]
>>> + def _get_key(self):
>>> + return self._key
>>
>> What do you need a private accessor for? Either it's private API and you
>> don't need an accessor (because your code knows it can access the
>> attribute directly) or it's public API. Even for public API, consider
>> whether it's likely that you need to do expensive calculations in order
>> to generate this value in the future. Apart from being local to the
>> sugar module (making it easy to adapt all callers), in Python you can
>> easily turn this into a property later on, so you only need an accessor
>> to indicate expensive accesses or side effects. See also PEP 8, section
>> "Naming Conventions", "Prescriptive: Naming Conventions", "Designing for
>> inheritance".
>>
>>
>> [...]
>>> +class KeyValuesDialog(gtk.Dialog):
>>> + def __init__(self, auth_lists, final_callback, uuid, settings):
>>> + # This must not be "modal", else the "chooser" widgets won't
>>> + # accept anything !!
>>
>> I'll also bite you if you introduce something modal. :-P
>>
>> IMO (Gary in particular will disagree) using a modal dialog always
>> implies that you made a design mistake somewhere. Why do you _need_ to
>> prevent the user from interacting with other parts of the system? How
>> can you be sure that preventing this interaction doesn't render the user
>> unable to complete the dialog successfully (e.g. by looking up the
>> passphrase somewhere)?
>>
>> In addition to being a design failure, modal dialogs are hard to get
>> right in practice. See e.g. SL#453 [5], SL#1138 [6]. I'm quite sure we
>> still haven't sorted out all issues with the existing modal dialogs.
>>
>> I also strongly dislike pop-up dialogs that the user hasn't explicitly
>> requested, but that's a topic for another day. :)
>>
>>
>> [src/jarabe/desktop/networkviews.py]
>>> @@ -56,6 +57,192 @@ _OLPC_MESH_ICON_NAME = 'network-mesh'
>>>
>>> _FILTERED_ALPHA = 0.33
>>>
>>> +SETTING_TYPE_STRING = 1
>>> +SETTING_TYPE_LIST = 2
>>> +SETTING_TYPE_CHOOSER = 3
>>
>> If these values need to be consistent across the different source files,
>> they should go into a single file. jarabe.model.network should be a good
>> fit.
>>
>>
>>> +class AuthenticationType:
>>> + def __init__(self, auth_label, auth_type, params_list):
>>
>> What is this all about? (include docstring, better class name, etc.)
>>
>>> + self._auth_label = auth_label
>>> + self._auth_type = auth_type
>>> + self._params_list = params_list
>>
>> Why does it have only private API, but apparently no subclass?
>>
>>
>>> +class AuthenticationParameter:
>>> + def __init__(self, key_name, key_label, key_type,
>>> + options):
>> [...]
>>
>> Ditto.
>>
>>
>>> +AUTHENTICATION_LIST = \
>>> + [
>>> + AuthenticationType('TLS',
>>> + 'tls',
>>> + [
>>> + AuthenticationParameter(
>>> + 'eap',
>>> + 'Authentication',
>>> + SETTING_TYPE_LIST,
>>> + [['TLS', 'tls']]
>>> + ),
>> [...]
>>
>> Still very hard to read and doesn't follow the PEP 8 indentation rules.
>>
>> Let's give it a try:
>>
>> WPA_AUTHENTICATION_METHODS = [
>> AuthenticationType(_('TLS'), 'tls',
>> [AuthenticationParameter('eap', _('Authentication'),
>> SETTING_TYPE_LIST,
>> [['TLS', 'tls']]),
>> AuthenticationParameter('identity', _('Identity'),
>> SETTING_TYPE_STRING, []),
>> AuthenticationParameter('client-cert',
>> _('User certificate'),
>> SETTING_TYPE_CHOOSER, []),
>> AuthenticationParameter('ca-cert', _('CA certificate'),
>> SETTING_TYPE_CHOOSER, []),
>> AuthenticationParameter('private-key',
>> _('Private key'),
>> SETTING_TYPE_CHOOSER, []),
>> AuthenticationParameter('private-key-password',
>> _('Private Key password'),
>> SETTING_TYPE_STRING, []),
>> ]),
>> AuthenticationType(_('LEAP'), 'leap',
>> [AuthenticationParameter('eap', _('Authentication'),
>> SETTING_TYPE_LIST,
>> [['LEAP', 'leap']]),
>>
>> and so on.
>>
>> Not perfect yet, but already much more compact. And it includes the
>> translated labels, solving your dynamic string translation issue from
>> above.
>>
>> With proper subclassing for the different value types it'll probably get
>> even better.
>>
>> I'm stopping the review at this point; the above should be enough to
>> chew on already.
>>
>> Sascha
>>
>> [1] http://www.python.org/dev/peps/pep-0008/
>> [2] https://wiki.sugarlabs.org/go/Development_Team/Code_Guidelines#Tools
>> [3] https://bugs.sugarlabs.org/ticket/2099
>> [4] https://bugs.sugarlabs.org/ticket/2023
>> [5] https://bugs.sugarlabs.org/ticket/453
>> [6] https://bugs.sugarlabs.org/ticket/1138
>> --
>> http://sascha.silbe.org/
>> http://www.infra-silbe.de/
>>
>> _______________________________________________
>> Dextrose mailing list
>> Dextrose at lists.sugarlabs.org
>> http://lists.sugarlabs.org/listinfo/dextrose
>>
More information about the Dextrose
mailing list