[Sugar-devel] [PATCH sugar] Control Panel: making about my computer section hardware independent, OLPC #11232

[Simon Schampijer]

On 10/09/2011 05:22 PM, Sascha Silbe wrote:
> Excerpts from Simon Schampijer's message of 2011-10-09 12:54:40 +0200:
>
>>>> @@ -58,6 +58,8 @@ def get_serial_number():
>>>>            serial_no = _read_file(os.path.join(_OFW_TREE, _SN))
>>>>        elif os.path.exists(os.path.join(_PROC_TREE, _SN)):
>>>>            serial_no = _read_file(os.path.join(_PROC_TREE, _SN))
>>>> +    else:
>>>> +        return None
>>>>        if serial_no is None:
>>>>            serial_no = _not_available
>>>>        return serial_no
>>>
>>> We are hiding the serial number completely if the kernel doesn't expose
>>> device tree information, but show "Not available" if the file couldn't
>>> be read or parsed. That doesn't really match the patch description.
>>>
>>> How about:
>>>
>>> def get_serial_number():
>>>       if os.path.exists(os.path.join(_OFW_TREE, _SN)):
>>>           return _read_file(os.path.join(_OFW_TREE, _SN))
>>>       elif os.path.exists(os.path.join(_PROC_TREE, _SN)):
>>>           return _read_file(os.path.join(_PROC_TREE, _SN))
>>>       elif os.path.exists(os.path.join(_DMI_DIRECTORY, 'board_serial'):
>>>           return _read_file(os.path.join(_DMI_DIRECTORY, 'board_serial'))
>>>       return None
>>
>> Ahh, maybe not as clear from the description. The board_serial is only
>> readable by root. Knowing that it does not really make sense to try to
>> read it.
>
> Ah, it's really just the board_serial that's readable by root only by
> default; I assumed the same was true for bios_version. In that case it
> makes sense not to bother with board_serial for now (i.e. until we have
> a downstream that makes board_serial readable for the user running Sugar).
>
> I suggest the following tweak to the description:
>
> The serial number is usually only readable by root on x86 based
> hardware, so we don't bother trying. If no serial number can be
> determined (for any reason) we hide that part of the section.
>
>
>> That is why I special cased displaying the serial number. The
>> behavior has not changed to previous code, so I think it is good like
>> that, espacially as a bug fix patch.
>
> Well, the behaviour does change for Firmware and Wireless Firmware.
> Before they were hidden, now they show 'Not available'. So why the
> special-casing just for the serial number?
>
> I don't care much either way (with a slight preference towards 'Not
> available'), but we should be consistent.
>
> Sascha

Ok, great _ I am d'accord with always displaying the entries, as well 
for the serial number. The new patch does that, good to push now?

Regards,
    Simon