3
\$\begingroup\$

After reading about large files and memory problems, I'm suspecting that my code below may be inefficient because it reads the entire files into memory before applying the hash algorithm. Is there a better way?

chunk_size = 1024
hasher = hashlib.md5()
while True:
    try:
        data = f.read(chunk_size)
    except IOError, e:
        log.error('error hashing %s on Agent %s' % (path, agent.name))
        return {'error': '%s' % e}
    if not data:
        break
    hasher.update(data)
hash_string = hasher.hexdigest()
\$\endgroup\$
1
\$\begingroup\$

Nope, exactly right, except that the chunk size should probably be bigger, typically page size, likely 4096 bytes, but that's cargo culted, so profiling would be better either way.

Also, it might be better to move the try/except block out of the loop, if just for readability. The return convention for errors is a bit weird, but since we don't know the context I can't comment more on that, except that '%s' % e should probably be str(e), because it's a bit shorter (and clearer IMO - string formatting should be used to format strings, not convert to string, but YMMV).

That said, try shelling out to md5sum $FILE and retrieve the result, might be faster; i.e. using subprocess.

\$\endgroup\$
3
\$\begingroup\$

Well, MD5 is considered to be mostly cracked, and should be avoided. But anyway…

The comma in except IOError, e is considered deprecated. except IOError as e is preferred since Python 2.6, and required in Python 3.x.

I don't think that your code is inefficient. It certainly doesn't read the entire file into memory at once. A larger chunk size wouldn't hurt. I think that 8192 bytes would be reasonable: it's approximately a memory page on some machines and approximately the size of a jumbo frame.

I think it would be stylistically beneficial to separate the file-reading code from the hash-calculating code using a generator. Alternatively, using mmap wouldn't be a bad idea.

def read_chunks(file_handle, chunk_size=8192):
    while True:
        data = file_handle.read(chunk_size)
        if not data:
            break
        yield data

def md5(file_handle):
    hasher = hashlib.md5()
    for chunk in read_chunks(file_handle):
        hasher.update(chunk)
    return hasher.hexdigest()

try:
    with open(sys.argv[1], 'rb') as f:
        hash_string = md5(f)
except IOError as e:
    log.error(…)
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.