4
\$\begingroup\$

This is a follow-up review request to the first one for this same utility/file/script. The first review can be found here: Cross-platform file hash sum generator in Python

I'm back with the modified version of this script/utility, looking for some more follow up on the original review.

This iteration of my hash sum script has applied multiple changes from the original one, taking into account the recommendations made and subsequent changes (for formatting of output or other things).

  • This version takes into account the concerns brought up about TOC-TOU, and I've removed any 'renaming of file path' cases, and any other potential TOC-TOU cases. (At least, I think so)
  • This version uses dicts for the digests, but also creates a dict to hold the hash sums after generation, so it can be formatted in output as needed.
  • No help / usage arguments. This is bad, I know, but the 'help' message is not ordering args correctly in the usage output, and every way to override has been breaking, so I have no way to fix that. (This used to have help/usage, but it doesn't work right anymore, so it was removed)
  • Yay for PEP8 styling! It's actually been applied this time with the recommended line lengths.

What am I looking for this time?

General review looking for more recommendations once again on improvements, but also I have concerns about speed of some of these hash sum generations.

Speed concern: MD5 sums

Running this on a 1.3GB file, the hash sums are generated correctly, as expected. However, for MD5 sums (I ran this script against the file with only MD5 sums specified), it consistently takes twice as long to generate as it does for other utilities in *nix system, which seems weird. This only applies to MD5 sums, it's comparable execution time for all other hashes supported here, so I'm not sure where the lagtime is coming from. If you have better recommendations for decreasing the time it takes to generate sums, I am open to them! Otherwise, I'm going to assume it's the fact this is Python and my approach that causes the substantial time difference for MD5 sums. (3 seconds with *nix md5sum utility, 6 seconds with this script`


Python Requirements

  • argparse, which may not be present on all systems.

No gist views this time, sorry!

compute-file-hashes.py:

#!/usr/bin/env python

import hashlib
import argparse

digests = {
    "md5": hashlib.md5,
    "sha1": hashlib.sha1,
    "sha256": hashlib.sha256,
    "sha512": hashlib.sha512,
}

# This code is licensed under the GNU General Public License (v3).
#
# The GNU GPLv3 can be read in full in the 'LICENSE' file, or at
# https://www.gnu.org/licenses/gpl-3.0.en.html.


# Customized exit handler, though we can't let it be called anywhere else.
def _exit(error_occurred=False, err=None):
    if error_occurred:
        print "\r\nProcess ended in error, details:"
        print _get_exception_string(err)
        exit(1)
    else:
        exit(0)


# Custom built-in for formatting Exception output in a friendly-ish way.
def _get_exception_string(exception=None):
    return "[%s] %s" % (type(exception).__name__, str(exception))


# Handler for hash computations, so we don't have to repeat the same 'hash it' code
# four times over
#
# noinspection PyUnusedLocal,PyShadowingNames
def _compute_hash(filepath, digest):
    try:
        filehash = digests[digest]()

        # Open the file specified in the filepath.
        with open(filepath, "rb") as filename:
            # Chunk file into 4096-byte chunks for processing. Automatically makes a single
            # chunk for processing if the file is less than 4096 bytes.
            for chunk in iter(lambda: filename.read(4096), b""):
                # For each chunk, incorporate the chunk into the hash sum.
                filehash.update(chunk)

        # Return the MD5 hash sum (hexadecimal) to the end user.
        return filehash.hexdigest()
    except KeyError:
        raise TypeError("Invalid digest type specified, accepted are: md5 , sha1, sha256, sha512")
    except Exception as e:
        raise RuntimeError(str(e))


def _get_arguments():
    # Argument Parser for the overall function
    parser = argparse.ArgumentParser(
        description="Run hash sum functions on a file and return them.", add_help=False)
    parser.add_argument('filepath',
                        help="Path to file to run sums on.")
    parser.add_argument('--hashes',
                        nargs='*',
                        choices=digests,
                        default=["md5", "sha1"],
                        help="Hashes to be used. Default: md5, sha1")
    return parser.parse_args()


def main():
    # Init "sums_to_run", which will contain what hash sums we should run.
    sums_to_run = []

    # First, we need to get the arguments, but the coder has a thing about cluttering up main(),
    # so they created a "getter" that will parse the arguments for us, and also return only the
    # arguments.
    args = _get_arguments()

    # Store the filepath argument.
    filepath = args.filepath

    # PreProcessing: Determine from args which functions to run.
    for algorithm in args.hashes:
        sums_to_run.append(algorithm)

    # Init an empty dict, in order for us to add key-value pairs of
    # hash algorithm and the actual hashes, which simplifies printing
    # later
    sums = {}

    try:
        # Provide some useful data to user about progress, and what file we're actually checking.
        print "FilePath: %s\r\n" % filepath

        for algorithm in sorted(digests.keys()):
            if algorithm in sums_to_run:
                print "Generating", algorithm.upper(), "sum, this could take some time..."
                sums[algorithm] = _compute_hash(filepath, algorithm)
                print algorithm.upper(), " hash calculated.\r\n"

        print "Done, output below.\r\n\r\n"
        print "FilePath: %s\r\n" % filepath
        print "Requested Hash Sums:"

        for algorithm in sorted(digests.keys()):
            if algorithm in sums_to_run:
                if algorithm == 'md5':
                    # Needs two spaces before for pretty alignment
                    print "  ", algorithm.upper(), "sum: %s" % str(sums[algorithm])
                elif algorithm == 'sha1':
                    # Needs one space before for pretty alignment
                    print " ", algorithm.upper(), "sum: %s" % str(sums[algorithm])
                else:
                    # Needs no spaces before for pretty alignment
                    print algorithm.upper(), "sum: %s" % str(sums[algorithm])

    except Exception as err:
        # If the process dies off for any reason, we need to catch the exception and state it back
        # to the user, then die off gracefully.
        print "Process died with error: %s" % str(err)
        _exit(error_occurred=True, err=err)

    else:
        _exit()

main()
\$\endgroup\$
3
\$\begingroup\$

Your list sums_to_run is completely unnecessary. Just use args.hashes. And if it was useful, copying would be better with sums_to_run = args.hashes[:].

Instead of writing:

for algorithm in sorted(digests.keys()):
    if algorithm in sums_to_run:
        ...

Just use:

for algorithm in sorted(args.hashes):
    ...

In _compute_hash the message for the user when he chooses an algorithm which is not in the list can be better written as

except KeyError:
    err_str = "Invalid digest type specified, accepted are: {}"
    raise TypeError(err_str.format(", ".join(digests)))

Note that I put the str in its own line, to have a line-length below 80 characters (another suggestion of PEP8).

For the printing of the algorithms and their results I would use the padding function of format:

max_len = max(len(key) for key in digests)
print "{:>{n}} sum: {}".format(algorithm.upper(), sums[algorithm], n=max_len)

This is basically a nested format, which first replaces the inner placeholder n to have the maximum length of any key of digest and then use that to right-align the algo name and give it appropriate padding.

Example:

>>> "{:>{n}} sum: {}".format("md5", 1234, n=6)
'   md5 sum: 1234'

You should guard the call to main() with a if __name__ == "__main__": clause to allow importing some parts of your code in another script.

\$\endgroup\$
  • \$\begingroup\$ +1 to your recommendations! I like the padding function of format, and that I can define the length as max_len, which I can add a number to it for additional padding on the output, which I like a bunch (currently running 2 extra spaces so it shows as 'indented' under the heading for generated sums). Thank you very much for your review! \$\endgroup\$ – Thomas Ward Aug 31 '16 at 17:22

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.