Re: [Autotest] [PATCH] KVM test: Add new program cd_hash.py

* Re: [Autotest] [PATCH] KVM test: Add new program cd_hash.py
       [not found] <1800286378.1167161256753565440.JavaMail.root@zmail05.collab.prod.int.phx2.redhat.com>
@ 2009-10-28 18:23 ` Michael Goldish
  2009-10-28 19:39   ` Lucas Meneghel Rodrigues
  0 siblings, 1 reply; 2+ messages in thread
From: Michael Goldish @ 2009-10-28 18:23 UTC (permalink / raw)
  To: Lucas Meneghel Rodrigues; +Cc: kvm, autotest

----- "Lucas Meneghel Rodrigues" <lmr@redhat.com> wrote:

> A new program that evaluates hash strings, intended
> to help kvm autotest administrators was added, cd_hash.
> 
> Usage: cd_hash.py [options]
> 
> Options:
>   -h, --help            show this help message and exit
>   -i FILENAME, --iso=FILENAME
>                         path to a ISO file whose hash string will be
>                         evaluated.
> 
> This script will calculate:
> 
>  * MD5SUM for the 1st MB of the file
>  * SHA1SUM for the 1st MB of the file
>  * MD5SUM for the whole file
>  * SHA1SUM for the whole file
> 
> The hashes for the 1st MB are calculated first in the case the
> user only wants them. This program replaces calc_md5sum_1m.
> 
> Signed-off-by: Lucas Meneghel Rodrigues <lmr@redhat.com>
> ---
>  client/tests/kvm/calc_md5sum_1m.py |   21 --------------
>  client/tests/kvm/cd_hash.py        |   54
> ++++++++++++++++++++++++++++++++++++
>  2 files changed, 54 insertions(+), 21 deletions(-)
>  delete mode 100755 client/tests/kvm/calc_md5sum_1m.py
>  create mode 100755 client/tests/kvm/cd_hash.py
> 
> diff --git a/client/tests/kvm/calc_md5sum_1m.py
> b/client/tests/kvm/calc_md5sum_1m.py
> deleted file mode 100755
> index 153a1e0..0000000
> --- a/client/tests/kvm/calc_md5sum_1m.py
> +++ /dev/null
> @@ -1,21 +0,0 @@
> -#!/usr/bin/python
> -"""
> -Program that calculates the md5sum for the first megabyte of a file.
> -It's faster than calculating the md5sum for the whole ISO image.
> -
> -@copyright: Red Hat 2008-2009
> -@author: Uri Lublin (uril@redhat.com)
> -"""
> -
> -import os, sys
> -import kvm_utils
> -
> -
> -if len(sys.argv) < 2:
> -    print 'usage: %s <iso-filename>' % sys.argv[0]
> -else:
> -    fname = sys.argv[1]
> -    if not os.access(fname, os.F_OK) or not os.access(fname,
> os.R_OK):
> -        print 'bad file name or permissions'
> -    else:
> -        print kvm_utils.hash_file(fname, 1024*1024, method="md5")
> diff --git a/client/tests/kvm/cd_hash.py
> b/client/tests/kvm/cd_hash.py
> new file mode 100755
> index 0000000..483d71c
> --- /dev/null
> +++ b/client/tests/kvm/cd_hash.py
> @@ -0,0 +1,54 @@
> +#!/usr/bin/python
> +"""
> +Program that calculates several hashes for a given CD image.
> +
> +@copyright: Red Hat 2008-2009
> +"""
> +
> +import os, sys, optparse, logging
> +import common, kvm_utils
> +from autotest_lib.client.common_lib import logging_config,
> logging_manager
> +
> +
> +class KvmLoggingConfig(logging_config.LoggingConfig):
> +    def configure_logging(self, results_dir=None, verbose=False):
> +        super(KvmLoggingConfig,
> self).configure_logging(use_console=True,
> +                                                       
> verbose=verbose)
> +
> +if __name__ == "__main__":
> +    parser = optparse.OptionParser()
> +    parser.add_option('-i', '--iso', type="string", dest="filename",
> +                      action='store',
> +                      help='path to a ISO file whose hash string will be '
> +                           'evaluated.')
> +
> +    options, args = parser.parse_args()
> +    filename = options.filename

I think it's a little annoying to have to use -i or --iso.
Why not just make the iso a mandatory argument that appears in 'args'?
If I understand correctly, all we need to do is change the last line to
'filename = args[0]' and modify the documentation accordingly.
But then if we only take a single argument, why use OptionParser at all?
The code will be simpler if we just look at sys.argv directly.

What do you think?

I'm having very similar thoughts about kvm_config.py, whose option parsing
I think is needlessly complex.

> +    logging_manager.configure_logging(KvmLoggingConfig())
> +
> +    if not filename:
> +        parser.print_help()
> +        sys.exit(1)
> +
> +    filename = os.path.abspath(filename)
> +
> +    file_exists = os.path.isfile(filename)
> +    can_read_file = os.access(filename, os.R_OK)
> +    if not file_exists:
> +        logging.critical("File %s does not exist. Aborting...",
> filename)
> +        sys.exit(1)
> +    if not can_read_file:
> +        logging.critical("File %s does not have read permissions. "
> +                         "Aborting...", filename)
> +        sys.exit(1)
> +
> +    logging.info("Hash values for file %s",
> os.path.basename(filename))
> +    logging.info("md5    (1m): %s", kvm_utils.hash_file(filename,
> 1024*1024,
> +                                                       
> method="md5"))
> +    logging.info("sha1   (1m): %s", kvm_utils.hash_file(filename,
> 1024*1024,
> +                                                       
> method="sha1"))
> +    logging.info("md5  (full): %s", kvm_utils.hash_file(filename,
> +                                                       
> method="md5"))
> +    logging.info("sha1 (full): %s", kvm_utils.hash_file(filename,
> +                                                       
> method="sha1"))
> -- 
> 1.6.2.5
> 
> _______________________________________________
> Autotest mailing list
> Autotest@test.kernel.org
> http://test.kernel.org/cgi-bin/mailman/listinfo/autotest

^ permalink raw reply	[flat|nested] 2+ messages in thread