[Cluster-devel] [PATCH 2/2] fence_amt: new fence agent for Intel AMT

[Marek Grac <mgrac@redhat.com>]

Hi Ondrej,

On 12/03/2013 04:28 PM, Ondrej Mular wrote:
> +def reboot_cycle(_, options):
> +    cmd = create_command(options, "cycle")
> +
> +    if options["log"] >= LOG_MODE_VERBOSE:
> +        options["debug_fh"].write("executing: " + cmd + "\n")
> +
> +    null = open('/dev/null', 'w')
> +    try:
> +        process = subprocess.Popen(cmd, stdout=null, stderr=null, shell=True)
> +    except OSError:
> +        null.close()
> +        fail_usage("Amttool not found or not accessible")
> +
I would suggest to stdout/stderr point to options["debug_fh"] so we can 
use this information.

> +def is_executable(path):
> +    if os.path.exists(path):
> +        stats = os.stat(path)
> +        if stat.S_ISREG(stats.st_mode) and os.access(path, os.X_OK):
> +            return True
> +    return False
imho this function is not needed because you have to check and control 
this after each subprocess.Popen() ; if you believe it is a good idea to 
have such function (I'm not totally against it) then put it in fencing 
library.

I have no other comments, good work

m,