File copy in Python for slow networks

Question

Please note: I have a follow up question with updated code, here.

I have written a file-copy routine, as I experienced unreliable results using standard Python when copying file over very slow networks.

The requirements are as follows:

• to be used on Windows only
• not to be limited by the 256 character limit of Windows
• configurable when to use hash checksum checking (source / target)
• configurable "buffer size" or "chunk size" (it seems that smaller chunks are more reliable on unreliable/slow networks)
• file only available on target file system AFTER all content has copied. If you copy files, the target filename already exists on the target filesystem, hence a remote application may "see" this file and try to open it while copy still in progress. This is implemented with a temp-target-filename in the code below.
• configurable console output

So this is what I have, and it tested to be working OK.

I am looking for tips/tricks to improve this code

PS, I know I haven't done "docstrings" yet... (on my to do list)

Usage:

import ccopy
source = r'c:\test1\testfile.ext'
target = r'\\someserver\test2\testfile.ext'
ccopy.filecopy(source, target, True, 'md4', 1024, True, True)

ccopy.py:

import os
import uuid
import hashlib
import platform

def normalizefilepath(path):
    #inserts '\\?\' at the start of a path to get rid of 256 character limit of Windows
    if path[1:3] == ':\\':
        return u'\\\\?\\' + os.path.normcase(path)
    return os.path.normcase(path)        

def hashfile(filepath, hashtype='md4', buffersize=1024):
    ha = hashlib.new(hashtype)
    f = open(filepath, 'rb')
    while True:
        chunk = f.read(buffersize)
        if not chunk: 
            break
        ha.update(chunk)
    f.close()
    return ha.digest() 

def filecopy(source, target, hashcheck = False, hashtype = 'md4', buffersize = 1024, overwrite = True, consoleoutput = False):   

    if not platform.system() == 'Windows':          raise Exception('Incorrect OS')
    if not hashtype in ['md4','md5', 'sha1']:       raise Exception('HashType must be ["md4" | "md5" | "sha1"]')
    if not os.path.exists(source):                  raise Exception('Source File not Found')
    if not isinstance(buffersize, int):             raise Exception('Buffer Size must be int')
    if buffersize > 1048576:                        raise Exception('Buffer Size > 1MB (1048576 bytes)')
    if os.path.isdir(target):                       raise Exception('Target must be a File not a Directory')
    if os.path.exists(target) and (not overwrite):  raise Exception('Target File exists')
    if not os.path.exists(os.path.dirname(target)): raise Exception('Target Directory does not exists')

    sourcedir = normalizefilepath(os.path.dirname(source))
    targetdir = normalizefilepath(os.path.dirname(target))

    sourcefilepath = normalizefilepath(source)
    targetfilepath = normalizefilepath(target)

    tempfilename = '.'.join([str(uuid.uuid4()), str(uuid.uuid4())])
    tempfilepath = os.path.join(targetdir, tempfilename)

    if hashcheck:
        if consoleoutput: print('Reading Source File / Calculating hashlib/' + hashtype)
        sourcefilehash = hashfile(sourcefilepath, hashtype, buffersize)
        if consoleoutput: print('Hashlib/' + hashtype + ' for Source File = ' + str(sourcefilehash))
        if consoleoutput: print()

    copysource = open(sourcefilepath, 'rb')
    copytarget = open(tempfilepath, 'wb') 
    if consoleoutput: print('Copying File')
    if consoleoutput: print('Source File     : ' + sourcefilepath)
    if consoleoutput: print('Temp Target File: ' + tempfilepath)
    if consoleoutput: print()
    while True:
        chunk = copysource.read(buffersize)
        if not chunk:
            break
        copytarget.write(chunk)
    copytarget.flush()
    os.fsync(copytarget.fileno())        
    copytarget.close()    
    copysource.close()
    if consoleoutput: print('File copied to Temp Target')
    if consoleoutput: print()

    if hashcheck:
        if consoleoutput: print('Reading Temp Target File / Calculating hashlib/' + hashtype)
        targetfilehash = hashfile(tempfilepath, hashtype, buffersize)
        if consoleoutput: print('Hashlib/' + hashtype + ' for Temp Target File = ' + str(sourcefilehash))
        if consoleoutput: print()        
        if not sourcefilehash == targetfilehash:
            if consoleoutput: print('Haslib/' + hashtype + ' mismatch') 
            if consoleoutput: print('Removing Temp Target File') 
            os.remove(tempfilepath)
            raise Exception('File not successfully copied: haslib/md4 mismatch')
        else:
            if consoleoutput: print('Source File and Temp Target File Hashlib/' + hashtype + ' match') 
            if consoleoutput: print()
            if consoleoutput: print('Renaming File') 
            if consoleoutput: print('Temp Target File: ' + tempfilepath) 
            if consoleoutput: print('Target File     : ' + targetfilepath) 
            os.replace(tempfilepath, targetfilepath)
    else:
        if consoleoutput: print('Renaming File') 
        if consoleoutput: print('Temp Target File: ' + tempfilepath) 
        if consoleoutput: print('Target File     : ' + targetfilepath) 
        os.replace(tempfilepath, targetfilepath)

def main():
    print('This script is only to be used by method of "import"')
    print('NOT to be used interactively')

if __name__ == '__main__':
    main()

Answer 1

I don't think there is much more to do/check in terms of functionality, but I'd like to point out a few coding style issues (in my opinion).

• Be careful when checking paths. If you are taking any kind of user input, keep in mind that .. is a valid path. You don't want people to use your module to copy files that they're not supposed to. If you're in a situation where you have to handle user-supplied paths, validate them AND use a whitelist to determine if they should be allowed to read the file. Have a look at abspath and/or realpath.

• I'd move all the checks you have at the beginning of filecopy in a separate function. You may need those same checks at a later time, you never know.

• You keep using if consoleoutput. This suggest that a function could be handy.

Maybe something like this:

def debug(messages):
    if (consoleoutput):
        for message in messages:
            print(message + '\n');

So the rest of the code could go from this:

if consoleoutput: print('Source File and Temp Target File Hashlib/' + hashtype + ' match') 
if consoleoutput: print()
if consoleoutput: print('Renaming File') 
if consoleoutput: print('Temp Target File: ' + tempfilepath) 
if consoleoutput: print('Target File     : ' + targetfilepath) 

to this:

    debug(['Source File and Temp Target File Hashlib/' + hashtype + ' match', 
        '', 
        'Renaming File', 
        'Temp Target File: ' + tempfilepath, 
        'Target File     : ' + targetfilepath])

Answer 2

Ok, here are my suggestions:

• Please don't munge variable names into one long string. Separate words with _ or use some variation of camelCasing. Names like normalizefilepath are difficult to read -- there aren't any breaks, so everything runs together. Names like normalize_file_path or normalizeFilePath are much easier to read.

• You include the following check:

  if not platform.system() == 'Windows': raise Exception('Incorrect OS')

  Is there a reason for this? Will the code really not work on any other platform other than Windows? Or are you just not supporting it? If the latter, I would consider raising a warning instead of an exception in this case. That way, you are declaring that the current platform is not supported (and thus might not work), but at the same time you do not outright prevent the code from functioning when it might work just fine.

• Your main() function just says not to use the file as a script. This function serves no purpose when the module is imported. So you effectively have a broken / useless function in your module. I would get rid of the function and just move that code to where you call main.