Allow sugar on non-XO hardware to register with an XS server

[hamiltonchua]

We would like to allow an SoaS to register with an XS. I have attached here diff files for schoolesrver.py and ds-backup.py that will hopefully allow this.

The modifications should still allow these scripts to work normally when installed in an XO.

Attached here are the diffs against the original files and the modified python scripts.

This is for review.

[daveb]

FOr this to work ds-backup-client needs the ds-backaup.py patch and ds-backup-client would need to be added to soas build i guess.

I think this is the right RPM (that needs the patched ds-backup.py)
 http://dev.laptop.org/~martin/public_rpms/joyride/ds-backup-client-0.8.1-1.olpc3.noarch.rpm

[daveb]

We found that storing the registration id in gconf is a problem because the cron job for backup can not access it.

We are working on a new patch that stores this in a file under ~/.sugar

[tomeu]

Please see this about submitting patches:

 http://wiki.sugarlabs.org/go/Development_Team/CodeReview#Patch_guidelines

Also would be good to have a separate ticket for each patch.

[martin.langhoff]

Hamilton, thanks for the patches. There are few issues however.

• I take it the diffs and the full files are equivalent, yes?

• Why do you remove -z from the rsynz invokation?

• Why do you set -a? In my testing, this will cause trouble as we want different mode on the XS.

• The code won't work on old Sugar. Better to make the gconf codepaths conditional, so we have a single ds-backup that works on both.

• I agree - gconf is a bad place to store info that may be needed from cron, have you got the new, updated patch?

• Can you make your diffs with '-u' so we get nice unified diffs? :-)

• The code in schoolserver.py that sets that SN/UUID should create them based on random values the first time it is called, and _store_ them. Right now you are tying the UUID to the MAC address of the machine. If SoaS is used as designed, the user may change the actual hardware on every run, effectively changing the MAC address. Not so good. Forget about the physical machine -- get a random value, make SN & UUID from it, store it, reuse it forevermore.

Put myself in CC.
~ cheers ~

[hamiltonchua]

Hello Martin,

Thanks for taking a look. Please see my responses below :

- I don't understand what you mean by the diff and full files being equivalent. I grabbed the original python scripts and diffed them against the modified scripts I made.

- I did not intentionally replace -a with -z. It may have been a typo on my part.

- I am replacing the gconf stuff. In order to use the values from gconf, a session has to be active. Because of this, the script when executed in a cron fails to get the uuid and serial. I have decided to store the values in files instead.

- Yes, my apologies, I will use -u next time.

- The MAC address taken from the current hardware the SoaS is plugged into is for purposes of generating a random UUID for registering into an XS. When it is plugged in again, the SoaS should already be registered and there is no need to check for the MAC address again. Is there a need to remember the random value used to generate the SN and UUID ? I think once we have an SN and UUID we don't need that random value anymore right ?

We are now testing the changes to both schoolserver.py and ds-backup.py where we store serial and UUID in files instead of gconf. Hope to have it posted here soon.

Thanks again.

[martin.langhoff]

Cool -- your comments make sense, thanks! Only one observation -- even if the MAC address is used only once (I misunderstood that!) it is a bad idea. Here is the scenario

• We have many laptops in the classroom, kids take one (anyone) and plug their SoaS in it.

• So user martin uses laptop A in his first day at school with his SoaS. His SN/UUID are made based on the MAC addr of laptop A....

• A week later, jimmy joins the group for the first time. Laptop A is the one he grabs! His SN/UUID are made based on the MAC addr of laptop A....

• Ooops!

[hamiltonchua]

Hi Martin

That's hilarious :-) Thanks !

Sorry, I didn't realize that that was the scenario you had in mind.

I had thought that maybe the extra random hexdigits at the end might be enough but I suppose why complicate things with getting the MAC Address and parsing it when you can just generate all 40 digits for the UUID anyway using random :-)

I'll include the change on my next update.

Thanks again.

Hamilton

[hamiltonchua]

Everyone,

I have uploaded new modified versions of ds-backup.py and schoolserver.py as well as patches you can apply to the original files.

We have been testing this and so far so good.

Please help us test further.

@martin.langhoff

The original ds-backup.py script from the rpm has one rsync using -a and another using -z. I have modified it so that they both use -z.

The UUID is now also generated purely using random hexdigits, it no longer users the mac address.

Also, I noticed that schoolserver.py stores the backup_url into gconf. In the ds-backup.py script the backup_url is hard coded. Backup seems to be working in spite of the hard code. Is there a need to modify it so that it reads the bacup_url given by the XS server ?

@tomeu

I would like to use this ticket to monitor the progress of this feature. The patch I created before will not work with the backup and restore when executed via cron so it must be discarded and not applied.

The new patches I have prepared changes the way we store the uuid and sn so that the backup script which runs via a cron can still retrieve these values.

[hamiltonchua]

I have uploaded a new patch for schoolserver.py. The register_url is now read from the jabber server field of the Network settings.

[hamiltonchua]

Please monitor progress with backup and restore on ticket #1124 ( http://dev.sugarlabs.org/ticket/1124)

[hamiltonchua]

Please note that I have attached a "git format-patch" and added the keyword "r?" so that the necessary modifications to schoolserver.py can be reviewed.

The attached schoolserver.py script is already patched and can be used to replace schoolserver.py on any SoaS install to enable it to register with an xs server.

Please disregard any file related to backups. I have created a separate ticket (#1124) for that which is also for review.

Thanks very much.

[hamiltonchua]

|TestCase|

Using a usb stick with 'Strawberry', copy the patched schoolserver.py to /usr/lib/python2.6/site-packages/jarabe/desktop.

Go to 'My Settings' and click 'Network'

Change the Jabber server to point to the XS server you want to register with.

Save and restart.

After restart, click the 'Register' menu item to register your stick.

Register should return a success message. To verify if all the needed information was retreived for backup and restore. Go to /home/liveuser/.sugar/. There should be a directory 'soas' with the following files

- sn
- uuid
- backup_url

[martin.langhoff]

My comments here echo my comments in #1124

• I would avoid hardcoding /home/liveuser/.sugar/soas - more notes on #1124

• I would avoid calling it SoaS :-) Yes, you are the primary user today, but "Sugar on non-XO hardware" is just Sugar. You are creating a useful identifier, so ~/.sugar/default/identifier (for UUID, SN) sounds like a better name. Same with functions.

[hamiltonchua]

I have updated schoolserver.py with Martin's suggestions. Please apply both patch 0001 and 0002.

[hamiltonchua]

If there are no objections, I'm changing the component from soas to sugar so that the modifications we have here can be incorporated to upstream Sugar as they can be useful when Sugar is used as a livecd or when installed on non-XO hardware.

[hamiltonchua]

If there are no objections, I'm changing the component from soas to sugar so that the modifications we have here can be incorporated to upstream Sugar as they can be useful when Sugar is used as a livecd or when installed on non-XO hardware.

[satellit]

have an opt out for booting if no school server is available.
either a time out or a keyboard key like <alt> at boot time
Use case:
1) student takes Soas v2 home and tries to use it.
2.) no network connection

[daveb]

Registering does not affect the boot process at all. It will boot and work perfectly well if it is temporarily disconnected from the XS.

[tomeu]

+import os,os.path

missing space

+# defs to enable registration of sugar to an xs on non-olpc hardware
+
+# utilities ********************************************************************

Please use API docs instead of inline comments. Explain what the code does, not how it does it. This applies to the whole patch.

+def getEpoch():

Don't use camel case, see how the rest of the code is written and try to do the same.

+# - we randomly get 3 letters
+# - concat the above with the last 8 numbers from epoch seconds

If you need to explain what the code does, it's because you can make the code clearer:

+  s1 = ''.join([random.choice(string.ascii_uppercase) for y in range(3)])
+  s2 = str(int(getEpoch()))[-8:]
+  serial = s1 + s2 
+  return serial

No need to make the code compact and hard to read, put a single expression in each line and use good names for each intermediate variable. This applies to the whole patch.

+def gen_identifier_serial():

Don't use abbreviations if at all possible. This applies to the whole patch.

+  u1_count = 40 

What's this magical value? May be better to have it as a constant at the module level, the name in all caps and with a leading underscore.

+  uuid = ''.join([random.choice(string.hexdigits + '-') for y in range(int(u1_count))])

Split this in several lines, please. This applies to the whole patch.

+        # logging.error('Registration: Cannot obtain data needed to register.')
+        # raise RegisterError(_('Cannot obtain data needed for registration.'))

Why this commented out code? If the code is not needed any more, please remove.

When you have a new patch, please do attach a single one that applies to the current git HEAD.

[erikos]

Best is to use 'git diff > name.patch' to generate the patch.

[hamiltonchua]

Thanks tomeu and erikos :-)

I have uploaded a new patched file of schoolserver.py and a new patch with all the changes against schoolserver.py from head.

To summarize the changes :
- I removed all the comments. I agree, the code needs to be understandable enough to not need them
- split up single expression short cuts into easier to understand multiple line expressions
- get rid of camel cases
- use the uuid module to generate a UUID string instead of trying to "reinvent the wheel"
- fix indentation

I'm resubmitting for review.

Thanks !

[tomeu]

Thanks, Hamilton, some more comments follow:

import os, os.path

No need to import os.path. Also see in  http://www.python.org/dev/peps/pep-0008/ how imports are to be grouped.

+def get_epoch():

Which epoch is that? Seems to me that what you want is the current time expressed in the number of seconds from the Unix epoch? If that's the case, time.time() should be enough.

+    t = datetime.datetime.now()
+    e = time.mktime(t.timetuple())  

Please use variable names that describe what the variables are for. This also applies to other parts of the patch.

+def create_identifier_serial():

What this function does? Maybe we can find a better name.

+def create_identifier_uuid():
+    uuid_string = uuid.uuid1()
+    uuid_string = str(uuid_string)
+    return uuid_string

Not sure it's worth a function for something that can be expressed clearly with str(uuid.uuid1()).

+def write_identifier_info(sn,uuid,backup_url):

Please see in  http://www.python.org/dev/peps/pep-0008/ about whitespace. This also applies to other parts of the patch.

Also, please put an empty line between the different parts of this function (maybe where you had comments before).

+    identifier_dir = os.path.expanduser('~') + '/.sugar/default/identifiers'

Please see this recent thread about this:  http://lists.sugarlabs.org/archive/sugar-devel/2009-August/017887.html

+        JABBER_SERVER = client.get_string('/desktop/sugar/collaboration/jabber_server')

We use all caps for static constants, dynamic constants are named as normal variables.

-    except (Error, socket.error):
-        logging.exception('Registration: cannot connect to server')
+    except (Error, socket.error), e:
+        logging.error('Registration: cannot connect to server: %s' % e)

Why doing this?

-        logging.error('Registration: server could not complete request: %s',
+        logging.error('Registration: server could not complete request: %s' % 

And this?

[hamiltonchua]

The changes to the exceptions and logging towards the end of the patch shouldn't be included, I have removed them.

I have changed JABBER_SERVER to jabber_server.

It now uses the env module from sugar to get the profile path.

I have renamed create_identifier_sn to generate_mfgdata_sn. This def generates a serial number based on 3 random uppercase letters and the last 8 digits of the current unix seconds.

I have removed the get_epoch def and used time.time() as you suggested. Thanks :-)

I have renamed create_identifier_uuid to generate_mfgdata_uuid. I simplified it further but decided to keep it as a separate function for consistency.

Thank you very much for your time and patience reviewing this patch. I really appreciate it.

I'm not sure what "r!" means in the Keywords field. I'm leaving it as is.

[tomeu]

+import string
+import random
+import time
+import uuid

These modules are part of the python std lib, please put them in the first group. If you could move gconf to a second group, would be great.

+def generate_mfgdata_sn():

mfgdata is an OLPC specific term and sn is an abbreviation that will confuse the casual reader, what about generate_serial_number()?

This def generates a serial number based on 3 random uppercase letters and the last 8 digits of the current unix seconds.

This probably belongs the API docs, not to this ticket.

+def generate_mfgdata_uuid():

I don't see with what this is consistent with, please remove the function.

+def store_mfgdata(sn, uuid, backup_url):

sn is an abbreviation that can mean several things, please use serial_number instead. Uuid is also an abbreviation but has become an accepted term of itself and would be really long to write the unabbreviated form.

+    backupurl_file = open(os.path.join(identifier_path, 'backup_url'),'w')

Separate words with _ for all identifiers.

Please check that you have spaces around operators and after commas.

Also, please run pylint and pep8.py and fix any messages, if in trouble ask in #sugar.

I'm not sure what "r!" means in the Keywords field. I'm leaving it as is.

Is meant to remove the ticket from the review queue, please set it to r? when you want another review. Have updated the wiki:

 http://wiki.sugarlabs.org/go/Development_Team/Code_Review#Discussion

[hamiltonchua]

@tomeu

Looks like I found a reason to keep generate_mfgdata_uuid. If I try :

uuid = str(uuid.uuid1())

I get an error

local variable 'uuid' referenced before assignment

any ideas ?

[sascha_silbe]

Replying to hamiltonchua:

uuid = str(uuid.uuid1())

uuid is a module. You try to overwrite it with the result of calling a function on this module. Even if pylint didn't complain (the error looks like a false positive to me) it would be a very bad idea. I suggest to rename your variable to something else, e.g. client_uuid or my_uuid.

[hamiltonchua]

@sascha_silbe
Thanks for the input. I just renamed the variable.

@tomeu
I updated schoolserver.py and register-non-xo-with-xs.patch.

- fixed indentation
- fixed missing spaces after some commas
- removed generate_mfgdata_uuid
- renamed uuid to uuid_ in register_laptop
- backupurl_file is now backup_url_file
- generate_mfgdata_sn is now generate_serial_number
- store_mfgdata is now store_identifiers

[tomeu]

Looks great, thanks!

This is ready to go in after:

    logging.error(identifier_path) 

Please remove this.

        url = 'http://'+jabber_server+':8080/' 

Spaces around operators.

The code is r+'ed but as the feature freeze has passed, you need to request an exception as explained here:  http://wiki.sugarlabs.org/go/Development_Team/Release#Feature_freeze

Please do so quickly because as time passes exceptions are harder to get.

[hamiltonchua]

Woohoo!!! Thanks Tomeu and thanks to everyone for their input.

- I updated schoolserver.py and the patch on this ticket
- removed the logging statement
- fixed spacing on the + operator

Will send the request for the exception now.

Thanks again !!!

[gregorio]

Hi Guys,

Is this resolved? Looks important for GPA implementation so I added that Keyword.

Nice collaboration and work on this tough one! If its fixed, call it done and close it.

Thanks,

Greg S

[tomeu]

Ooops, yes it went in here:

 http://git.sugarlabs.org/projects/sugar/repos/mainline/commits/6c832e5681