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

[Sascha Silbe]

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

-- 
http://sascha.silbe.org/
http://www.infra-silbe.de/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://lists.sugarlabs.org/archive/sugar-devel/attachments/20111009/9729b0ac/attachment.pgp>