[Server-devel] [PATCH] Touch a ".transfer_complete" to mark completion, minor cleanups

[Martin Langhoff]

On Mon, Jun 16, 2008 at 4:42 PM, Michael Stone <michael at laptop.org> wrote:
>> -def new_backup_notify(server, nonce, xo_serial):
>> -    try:
>> -        auth = sha.sha(nonce + xo_serial)
>> -        # TODO: add auth header
>> -        ret = urllib.urlopen(server + '/new/%s' % xo_serial).read()
>> -    except IOError, e:
>> -        if e[1] == 403:
>> -            # Auth not accepted. Shouldn't normally happen.
>> -            raise BackupError(server)
>> -
>
> Why are we taking out this part of Ivan's protocol? As I recall, this
> part of the protocol is important for mitigating DoS attacks on the
> backup server...

Not taking it out. This section removed is repeated again in
find_last_backup(). Before this patch, the backup script assumed that
"first" backup was special -- hence the "/new/" url - but this is no
longer the case. My (unfinished) task includes revamping what I left
(still in find_last_backup()) and refactor it. It won't try to compute
the list of files on its own, it will merely authenticate & announce
to the server that we intend to rsync, and see the reaction from the
XS (as the removed block does).

In any case, all of this can merely prevent a "friendly fire" DoS. For
true DoS protection from clients we need an rsync wrapper - relatively
easy to do, a bit harder to secure. We'll need rssh to lock down
things in this area, and as far as I can see, rssh ain't too friendly
with wrappers in high-level languages.

>>  def rsync_to_xs(from_path, to_path, keyfile, user):
>>
>>      # add a trailing slash to ensure
>> -    rsync = """/usr/bin/rsync -az --partial --delete --timeout=160 -e '%s' '%s' '%s' """ % \
>> +    rsync = "/usr/bin/rsync -az --partial --delete --timeout=160 -e '%s' '%s' '%s' " % \
> ...
>>      rsync_p = popen2.Popen3(rsync, True)
> ...
>> -    rsync_exit = rsync_p.wait()
>> +    rsync_exit = os.WEXITSTATUS(rsync_p.wait())
>> +    if rsync_exit != 0:
>> +        # TODO: retry a couple ofd times
>> +        # if rsync_exit is 30 (Timeout in data send/receive)
>> +        raise TransferError('rsync error code %s, message:'
>> +                            % rsync_exit, rsync_p.childerr.read())
>
> Since we're waiting for rsync to complete, is there any reason we aren't
> using subprocess.check_call() to wrap all this up in a bow tie?

Is subprocess.check_call() a better idiom? Where can I find good examples?
At times I am guilty of cargo-cultism in my Python.Show me the good
gospel, and I'll be a convert.

>> +    # Transfer an empty file marking completion
>> +    # so the XS can see we are done.
>> +    tmpfile = tempfile.mkstemp()
>
> Any reason to leave the fd returned as the first component
> tempfile.mkstemp() open?

You want to close it? Ok, makes sense.

> Also, do we delete the tempfile when we're done with it?

No. Good point - stupid me, I think I was meaning create it in the
.sugar/default dir to later mv it to be the "backup-done" flag.

cheers,



m
-- 
 martin at laptop.org -- School Server Architect
 - ask interesting questions
 - don't get distracted with shiny stuff - working code first
 - http://wiki.laptop.org/go/User:Martinlanghoff