From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Grac <mgrac@redhat.com>
Date: Thu, 19 Dec 2013 16:40:21 +0100
Subject: [Cluster-devel] [PATCH 2/2] fence_amt: new fence agent for
 Intel AMT
In-Reply-To: <1907891632.3279962.1386084496717.JavaMail.root@redhat.com>
References: <1907891632.3279962.1386084496717.JavaMail.root@redhat.com>
Message-ID: <52B31365.8070201@redhat.com>
List-Id: <cluster-devel.redhat.com>
To: cluster-devel.redhat.com
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit

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,