在许多磁盘上执行智能测试

Question

总的来说，我对编程还比较陌生，我终于开始学习Python了。这是我使用它的第一个重要项目，我正在尝试通过代码布局、可读性、注释、何时创建函数、何时不使用等来建立良好的习惯。

我正在解决的问题是，我有大约20个硬盘驱动器，我想在我的ubuntu服务器系统上运行相同的测试，并在将它们放入集群系统之前，生成详细说明它们的状态/健康情况的好文件。

我的一些具体关切是：

1. 整体工作流程。我是否正确地实现了我的main()函数？我想出了一些在main()之外的脚本之外可以使用的函数，这样我就可以在其他脚本中调用它们。但在里面留下了一些main()特有的东西。
2. 评论。更多?较少?这只对我来说是暂时的，但是你能看到发生了什么吗？
3. 我做了很多字符串操作，在我看来，这看起来有点混乱。有没有更干净/更简单的方法来做到这一点？

欢迎任何其他评论！

#!/usr/bin/python
##----------------
##Script Name: hd-diag
##Description of Script: Runs diagnostics on harddrives and writes results to
##file
##Date: 2013.07.09-11:48
##Version: 0.1.0
##Requirements: python2, smartmontools, posix environment
##TODO: add support for other incorrect input in YN questions
##----------------

import os
import sys
import subprocess
import getopt


def unix_call(cmd):
    '''(str) -> str

    Calls unix terminal process cmd and returns stdout upon process completion.
    '''

    output = []
    global errordic
    p = subprocess.Popen(
        cmd, shell=True,
        stdout=subprocess.PIPE, stderr=subprocess.PIPE)
    for line in iter(p.stdout.readline, ''):
        output.append(line.strip('\n'))
    if p.stderr:
        for line in iter(p.stderr.readline, ''):
            errordic['unix_call'] = line.strip('\n')
    return '\n'.join(output).strip("'[]'")


def test_call(cmd):
    p = subprocess.Popen(
        cmd, shell=True, stdout=subprocess.PIPE)
    test_results_raw = p.stdout.read()
    return test_results_raw


def write_str_to_file(string, filenamepath):
    '''(str) -> string in file

    Writes the contents of str to file as array. Assumes current directory
    if just filename is given.

    Full path must be given.
    '''

    def do_write(string, destination):
        to_file = open(destination, 'w')
        for item in string:
            to_file.write(str(item))
        to_file.close()

    if '/' not in filenamepath:
        cur_dir = os.getcwd()
        dest = cur_dir + '/' + filenamepath
        do_write(string, dest)
    elif '/' in filenamepath:
        if filenamepath[0] != '/':
            dest = '/' + filenamepath
            do_write(string, dest)
        elif filenamepath[0] == '/':
            do_write(string, filenamepath)


#set global variables
errordic = {}
driveloc = ''
outputloc = ''


def main():
    global driveloc, outputloc, errordic

    def getargs(argv):
        global driveloc, outputloc, errordic
        try:
            opts, args = getopt.getopt(
                argv, 'hd:o:', ['driveloc=', 'outputloc='])
        except getopt.GetoptError:
            usage()
            sys.exit(2)
        for opt, arg in opts:
            if opt == '-h':
                usage()
                sys.exit()
            elif opt in ('-d', '--driveloc'):
                driveloc = arg
            elif opt in ('-o', '--outputloc'):
                outputloc = arg

    def usage():
        print "hd-diag.py -d <hdpath> -o <outputloc>"

    getargs(sys.argv[1:])

    print "Selected drive is: ", driveloc
    if outputloc != '':
        if outputloc[-1] != '/':
            outputloc = outputloc + '/'
            print "File output location is: ", outputloc
        elif outputloc[-1] == '/':
            print "File output location is: ", outputloc
    elif outputloc == '':
        print "No output location selected. Using current directory."
        outputloc = os.getcwd() + '/'
        print "File output location is: ", outputloc
    #TODO: detect here if drive is ata or sata and print

    #make sure harddrive is unmounted
    while unix_call("mount | grep '%s'" % driveloc):
        unix_call("sudo umount %s" % driveloc)
        if unix_call("mount | grep '%s'" % driveloc):
            print"Can't unmount %s" % driveloc
            print"Err:", errordic['unix_call']
            print"Exiting."
            sys.exit(1)
    else:
        print"%s is not mounted, continuing" % driveloc

    #check if drive supports SMART capability, enabled if need be
    if 'Available' in unix_call(
            "sudo smartctl -i %s | grep 'SMART support is:'" % driveloc):
        print "SMART support is available on %s..." % driveloc
        if 'Enabled' in unix_call(
                "sudo smartctl -i %s | grep 'SMART support is:'" % driveloc):
            print "...and enabled"
        else:
            print "...but disabled"
            while not 'Enabled' in unix_call("smartctl -i %s" % driveloc):
                enableYN = raw_input(
                    "Would you like to enable SMART support? [y/n]:")
                if enableYN in ('yYes'):
                    unix_call("sudo smartctl -s %s" % driveloc)
                elif enableYN in ('nNo'):
                    print "Script cannot continue without SMART capability "
                    "enabled"
                    print "Exiting."
                    sys.exit(1)
                else:
                    print 'Sorry, but "%s" is not a valid input.' % enableYN

    #run desired tests on harddrive
    #get estimated time for extended test
    est_ttime = unix_call(
        "sudo smartctl -c %s | grep -A 1 'Extended self-test'" % driveloc)
    est_ttime = est_ttime[est_ttime.rfind('('):].replace('( ', '(').rstrip('.')
    testYN = raw_input("The estimated time for the test is %s, would you "
                       "like to proceed?[y/n]: " % est_ttime)
    #run test
    if testYN in ('Yy'):
        console_tstart = test_call("sudo smartctl -t long %s" % driveloc)
        print console_tstart
    elif testYN in ('Nn'):
        print 'Test cancelled. Exiting.'
        sys.exit(1)

    #prompt for continue
    contCE = raw_input(
        "\nThe test is running. After the estimated time listed "
        "above has passed, press 'c' to coninue or type 'exit' to "
        "exit.[c/exit]: ")
    #extract test result data
    if contCE in 'cC':
        console_tinfo = test_call(
            "sudo smartctl -i %s" % driveloc).split('\n', 2)
        console_thealth = test_call(
            "sudo smartctl -H %s" % driveloc).split('\n', 2)
        console_tresults = test_call(
            "sudo smartctl -l selftest %s" % driveloc).split('\n', 2)
        console_terror = test_call(
            "sudo smartctl -l error %s" % driveloc).split('\n', 2)
        log_tinfo = str(console_tinfo[2]).lstrip('\n')
        log_thealth = str(console_thealth[2]).lstrip('\n')
        log_tresults = str(console_tresults[2]).lstrip('\n')
        log_terror = str(console_terror[2]).lstrip('\n')
        print log_tinfo + log_thealth + log_tresults + log_terror
    elif contCE in 'Exitexit':
        print 'Test cancelled. Exiting.'
        sys.exit(1)

    #write output to file
    #extract brand, model, serial no., and size for file naming
    deviceinfo = [i.strip('\n').lstrip() for i in test_call(
        "sudo smartctl -i %s" % driveloc).split(':')]
    devicebrand = deviceinfo[2].split(' ')[0]
    devicemodel = deviceinfo[3].split(' ')[0].split('\n')[0]
    deviceserial = deviceinfo[4].split(' ')[0].split('\n')[0]
    devicesize = deviceinfo[6].split(' ')[2].strip('[')
    filename = '-'.join([devicebrand, devicemodel, deviceserial, devicesize])

    #write data to file
    finalwrite = outputloc + filename
    write_confirm = raw_input(
        "The data is ready to be written to:\n\t%s\n\n"
        "would you like to continue?[y/n]: " % finalwrite)
    if write_confirm in 'Yy':
        #TODO test for already existing file
        write_str_to_file(
            log_tinfo + log_thealth + log_tresults + log_terror, finalwrite)
        print "File writen. Test complete. Exiting"
        sys.exit(1)
    elif write_confirm in 'Nn':
        print "File write canceled. Exiting"
        sys.exit(1)

#main
if __name__ == '__main__':
    main()

Answer 1

查看您编辑的代码：

( a)在：

    output = []
    global errordic
    p = subprocess.Popen(
        cmd, shell=True,
        stdout=subprocess.PIPE, stderr=subprocess.PIPE)
    for line in iter(p.stdout.readline, ''):
        output.append(line.strip('\n'))
    if p.stderr:
        for line in iter(p.stderr.readline, ''):
            errordic['unix_call'] = line.strip('\n')
    return '\n'.join(output).strip("'[]'")

1)使用output的方式似乎太复杂了:如果您想这样做的话：

a=[]
for b in c
    a.append(f(b))

Python为您提供了一种使用列表理解进行此操作的良好而高效的方法：a=[f(b) for b in c]。

在你的例子中，你可以写：

return '\n'.join([l.strip('\n') for l in iter(p.stdout.readline,'')]).strip("'[]'")

但我不太清楚为什么您删除换行符后才将它们放回原处。

2)填充errordic['unix_call']的方式也很奇怪，因为您一直在以这样的方式替换值，在最后，您只有最新的行。假设这是错误的，而且我理解你想要做的事情，那么清单理解可以再次帮助你：

errordic['unix_call'] = '\n'.join(l.strip('\n') for l in iter(p.stderr.readline, ''))

现在，这将被验证，但我希望我的2段代码(大致)与以下代码相同：

errordic['unix_call'] = ''.join(iter(p.stderr.readline, ''))
return ''.join(iter(p.stdout.readline,'')).strip("'[]'")

另外，您可以返回一对字符串(stdout和stderr)，而不是以一种奇怪的方式使用全局变量。

然后，你是否需要unix_call和test_call，尽管它们看起来非常相似？

( c)在

if path.lower() == 'current':
    new_path = os.getcwd()
else:
    if os.sep not in path:  # unrecognized filepath
        new_path = os.getcwd()
    else:
        if path[0] != os.sep:
            new_path = os.path.join(os.sep, path)
        else:
            new_path = path

你可以让我的使用更加直截了当

if (path.lower() == 'current' or os.sep not in path):
    new_path = os.getcwd()
elif path[0] == os.sep:
    new_path = path
else:
    new_path = os.path.join(os.sep, path)

( d)这：

if filename.lower() == 'null':
    new_filename = ''
else:
    new_filename = filename

可以是一个简单的：

new_filename = '' if (filename.lower() == 'null') else filename

( e)这一点：

if outputloc != '':
    outputloc = file_path_check(outputloc, 'null')
    print "File output location is: ", outputloc
else:
    print "No output location selected. Using current directory."
    outputloc = file_path_check('current', 'null')
    print "File output location is: ", outputloc

可以是：

if outputloc == '':
    print "No output location selected. Using current directory."
    outputloc = 'current'
outputloc = file_path_check(outputloc, 'null')
print "File output location is: ", outputloc

( f)这一点：

#make sure drive is unmounted
if not unix_call("mount | grep '%s'" % driveloc):
    print "%s is not mounted, continuing" % driveloc
else:
    while unix_call("mount | grep '%s'" % driveloc):
        unix_call("sudo umount %s" % driveloc)
        if unix_call("mount | grep '%s'" % driveloc):
            print"Can't unmount %s" % driveloc
            print"Err:", errordic['unix_call']
            print"Exiting."
            sys.exit(2)

可以写成：

#make sure drive is unmounted
while unix_call("mount | grep '%s'" % driveloc):
    unix_call("sudo umount %s" % driveloc)
    if unix_call("mount | grep '%s'" % driveloc):
        print"Can't unmount %s" % driveloc
        print"Err:", errordic['unix_call']
        print"Exiting."
        sys.exit(2)
print "%s is not mounted, continuing" % driveloc

( g)可能值得定义一个以字符串作为参数的方法，将其提示给用户，并向他询问y/n的答案，直到给出一个适当的值并返回一个布尔值。

Answer 2

不要像这样打开文件。这是个坏习惯。忘记关闭它是很容易的。

to_file = open(destination, 'w')

用它做这个。

def do_write(string, destination):
    with open(destination, 'w') as to_file: 
        for item in string:
            to_file.write(item)

另外，当您传递一个字符串时，就不需要再次使用str了。多余的。

您正在进行冗余的elif检查。显然，if '/' not in filenamepath:是False，所以您不需要检查它是否在filepath中。使用else代替。

使用os.path.join代替字符串操作来连接路径。它是独立的。另外，检查/也不是一个好主意。您可以使用os.sep，这也是与os无关的。但是，确保filename本身不包含/是一个好主意。

在注释中，您说必须给出完整的路径，但是在函数中，您正在检查这一点。为什么？决定你想让这个函数做什么。两者都有？然后你可以用这个

def write_str_to_file(string, filenamepath):
    '''Writes the contents of str to file as array.

    Assumes current directory if just filename is given
    otherwise full path must be given
    '''
    if os.sep not in filenamepath:
        dest = os.path.join(os.getcwd(), filenamepath)
    else:
        dest = filenamepath

    with open(dest, 'w') as to_file:
        for item in string:
            to_file.write(str(item))

不要在不确定的地方定义全局变量。很难找到他们。把所有的球体放在顶端。如果没有必要，不要使用全局变量。在main函数中声明它们并传递它们。您可以返回多个值，因此在传递和返回所有值时会出现问题。当您不需要全局变量时，它们会显示出糟糕的设计。有时这可能是必要的，但在这种情况下似乎并不是必要的。

对于test_results_raw，您不需要额外的变量test_call。你可以用

def test_call(cmd):
    p = subprocess.Popen(
        cmd, shell=True, stdout=subprocess.PIPE)
    return p.stdout.read()

是usage函数中实际必需的函数main。您可以在模块的开头定义一个常量，然后将其打印出来。函数调用很昂贵，对1 print语句执行函数调用似乎不是个好主意。

同样在main函数中，您使用的是if outputloc != ''，然后是elif outputloc == ''。它可以是一个简单的else。嵌套的if和elif也是如此。检查是否有更多的条件会降低您的性能。

在检查enableYN时，可以使用enable.lower()[0] == 'y'而不是enableYN in ('yYes')。在这种情况下，它也会检查e和s。人们将进入y，Y，yes或其他一些变体，但在开始时使用y。检查是否没有也是一样的。

我跳过了很多代码，因为它很大。

希望这能帮上忙。

我能从评论中看到发生了什么。不怎么有意思。您错过了main函数正在做的事情。

琴弦乱七八糟。是的，他们是。试着改变我提到的事情，我会再看一遍。添加更新的代码，就像在这个问题中完成的那样，而不是修改它。

Answer 3

你为什么要用错码？如果您想让一个函数返回到变量，只需这样做。

def one_two():
    return 1, 2
one, two = one_two

subprocess.communicate似乎也能做你想做的事。

test_call似乎是subprocess.check_output的克隆。