Re: [Freeipa-devel] [PATCH] 0091 ipautil.run: Log the command line before running the command
Jan Cholasta wrote:
On 16.10.2012 17:26, Petr Viktorin wrote:
On 10/16/2012 04:53 PM, Jan Cholasta wrote:
On 16.10.2012 16:27, Petr Viktorin wrote:
On 10/16/2012 04:02 PM, Jan Cholasta wrote:
Hi,
On 15.10.2012 14:45, Petr Viktorin wrote:
As I was debugging code that calls long-running or failing
commands, I
got tired of the invocation being logged after the command is done.
This patch should improve the logging.
https://fedorahosted.org/freeipa/ticket/3174
---
Petr³
+except:
+root_logger.debug('Process failed')
+raise
Why do you use blank except here? Can you print the reason why the
process has failed or the exit code here?
Honza
I re-raise command afterwards, so the error is not really handled or
ignored. It's one of the few situations where a bare except is
justified :)
OK, makes sense.
Since the error is re-raised, it should be handled by the caller.
Printing additional info here should be redundant.
The exception can happen before the Popen object is created, so the
return code might not be available. Anyway bad return code is handled
later, here we'd get errors like command not found, fork() failure etc.
Right. Anyway, I think logging the return code might come handy in some
situations. Can you please add it?
You're right. Added.
I see that the wording could be better, attaching new patch:
s/Process failed/Process execution failed/
s/Process successful/Process finished/
Honza
Thanks. ACK.
Honza
pushed to ipa-3-0 and master
rob
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0091 ipautil.run: Log the command line before running the command
On 16.10.2012 16:27, Petr Viktorin wrote:
On 10/16/2012 04:02 PM, Jan Cholasta wrote:
Hi,
On 15.10.2012 14:45, Petr Viktorin wrote:
As I was debugging code that calls long-running or failing commands, I
got tired of the invocation being logged after the command is done.
This patch should improve the logging.
https://fedorahosted.org/freeipa/ticket/3174
---
Petr³
+except:
+root_logger.debug('Process failed')
+raise
Why do you use blank except here? Can you print the reason why the
process has failed or the exit code here?
Honza
I re-raise command afterwards, so the error is not really handled or
ignored. It's one of the few situations where a bare except is justified :)
OK, makes sense.
Since the error is re-raised, it should be handled by the caller.
Printing additional info here should be redundant.
The exception can happen before the Popen object is created, so the
return code might not be available. Anyway bad return code is handled
later, here we'd get errors like command not found, fork() failure etc.
Right. Anyway, I think logging the return code might come handy in some
situations. Can you please add it?
I see that the wording could be better, attaching new patch:
s/Process failed/Process execution failed/
s/Process successful/Process finished/
Honza
--
Jan Cholasta
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0091 ipautil.run: Log the command line before running the command
On 16.10.2012 17:26, Petr Viktorin wrote:
On 10/16/2012 04:53 PM, Jan Cholasta wrote:
On 16.10.2012 16:27, Petr Viktorin wrote:
On 10/16/2012 04:02 PM, Jan Cholasta wrote:
Hi,
On 15.10.2012 14:45, Petr Viktorin wrote:
As I was debugging code that calls long-running or failing commands, I
got tired of the invocation being logged after the command is done.
This patch should improve the logging.
https://fedorahosted.org/freeipa/ticket/3174
---
Petr³
+except:
+root_logger.debug('Process failed')
+raise
Why do you use blank except here? Can you print the reason why the
process has failed or the exit code here?
Honza
I re-raise command afterwards, so the error is not really handled or
ignored. It's one of the few situations where a bare except is
justified :)
OK, makes sense.
Since the error is re-raised, it should be handled by the caller.
Printing additional info here should be redundant.
The exception can happen before the Popen object is created, so the
return code might not be available. Anyway bad return code is handled
later, here we'd get errors like command not found, fork() failure etc.
Right. Anyway, I think logging the return code might come handy in some
situations. Can you please add it?
You're right. Added.
I see that the wording could be better, attaching new patch:
s/Process failed/Process execution failed/
s/Process successful/Process finished/
Honza
Thanks. ACK.
Honza
--
Jan Cholasta
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0091 ipautil.run: Log the command line before running the command
On 10/16/2012 04:53 PM, Jan Cholasta wrote:
On 16.10.2012 16:27, Petr Viktorin wrote:
On 10/16/2012 04:02 PM, Jan Cholasta wrote:
Hi,
On 15.10.2012 14:45, Petr Viktorin wrote:
As I was debugging code that calls long-running or failing commands, I
got tired of the invocation being logged after the command is done.
This patch should improve the logging.
https://fedorahosted.org/freeipa/ticket/3174
---
Petr³
+except:
+root_logger.debug('Process failed')
+raise
Why do you use blank except here? Can you print the reason why the
process has failed or the exit code here?
Honza
I re-raise command afterwards, so the error is not really handled or
ignored. It's one of the few situations where a bare except is
justified :)
OK, makes sense.
Since the error is re-raised, it should be handled by the caller.
Printing additional info here should be redundant.
The exception can happen before the Popen object is created, so the
return code might not be available. Anyway bad return code is handled
later, here we'd get errors like command not found, fork() failure etc.
Right. Anyway, I think logging the return code might come handy in some
situations. Can you please add it?
You're right. Added.
I see that the wording could be better, attaching new patch:
s/Process failed/Process execution failed/
s/Process successful/Process finished/
Honza
--
Petr³
From 41f6557a1f7712316e297c1221e12f251c53e906 Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Tue, 25 Sep 2012 09:29:49 -0400
Subject: [PATCH] ipautil.run: Log the command line before running the command
When the user interrupts a long-running command, this ensures that
the command is logged. Also, when watching log files (or the -d
output), it's apparent what's being done.
https://fedorahosted.org/freeipa/ticket/3174
---
ipapython/ipautil.py | 17 +++--
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py
index 0b519c2957f63770f9a28d7abe9083f724a9cf40..e76d87d3dfcabc473f833d566cf919f95ff2f68e 100644
--- a/ipapython/ipautil.py
+++ b/ipapython/ipautil.py
@@ -292,30 +292,35 @@ def run(args, stdin=None, raiseonerr=True,
p_out = subprocess.PIPE
p_err = subprocess.PIPE
+arg_string = nolog_replace(' '.join(args), nolog)
+root_logger.debug('Starting external process')
+root_logger.debug('args=%s' % arg_string)
+
try:
p = subprocess.Popen(args, stdin=p_in, stdout=p_out, stderr=p_err,
close_fds=True, env=env, cwd=cwd)
stdout,stderr = p.communicate(stdin)
stdout,stderr = str(stdout), str(stderr)# Make pylint happy
except KeyboardInterrupt:
+root_logger.debug('Process interrupted')
p.wait()
raise
+except:
+root_logger.debug('Process execution failed')
+raise
+
+root_logger.debug('Process finished, return code=%s', p.returncode)
# The command and its output may include passwords that we don't want
# to log. Replace those.
-args = ' '.join(args)
if capture_output:
stdout = nolog_replace(stdout, nolog)
stderr = nolog_replace(stderr, nolog)
-args = nolog_replace(args, nolog)
-
-root_logger.debug('args=%s' % args)
-if capture_output:
root_logger.debug('stdout=%s' % stdout)
root_logger.debug('stderr=%s' % stderr)
if p.returncode != 0 and raiseonerr:
-raise CalledProcessError(p.returncode, args)
+raise CalledProcessError(p.returncode, arg_string)
return (stdout, stderr, p.returncode)
--
1.7.11.7
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 0091 ipautil.run: Log the command line before running the command
As I was debugging code that calls long-running or failing commands, I
got tired of the invocation being logged after the command is done.
This patch should improve the logging.
https://fedorahosted.org/freeipa/ticket/3174
---
Petr³
From dd504f133857d310938ba3c43065485ee90b6073 Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Tue, 25 Sep 2012 09:29:49 -0400
Subject: [PATCH] ipautil.run: Log the command line before running the command
When the user interrupts a long-running command, this ensures that
the command is logged. Also, when watching log files (or the -d
output), it's apparent what's being done.
https://fedorahosted.org/freeipa/ticket/3174
---
ipapython/ipautil.py | 17 +++--
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py
index 0b519c2957f63770f9a28d7abe9083f724a9cf40..aa86f7557be0d90ac7ca7298aa0c5023bd776696 100644
--- a/ipapython/ipautil.py
+++ b/ipapython/ipautil.py
@@ -292,30 +292,35 @@ def run(args, stdin=None, raiseonerr=True,
p_out = subprocess.PIPE
p_err = subprocess.PIPE
+arg_string = nolog_replace(' '.join(args), nolog)
+root_logger.debug('Starting external process')
+root_logger.debug('args=%s' % arg_string)
+
try:
p = subprocess.Popen(args, stdin=p_in, stdout=p_out, stderr=p_err,
close_fds=True, env=env, cwd=cwd)
stdout,stderr = p.communicate(stdin)
stdout,stderr = str(stdout), str(stderr)# Make pylint happy
except KeyboardInterrupt:
+root_logger.debug('Process interrupted')
p.wait()
raise
+except:
+root_logger.debug('Process failed')
+raise
+
+root_logger.debug('Process successful')
# The command and its output may include passwords that we don't want
# to log. Replace those.
-args = ' '.join(args)
if capture_output:
stdout = nolog_replace(stdout, nolog)
stderr = nolog_replace(stderr, nolog)
-args = nolog_replace(args, nolog)
-
-root_logger.debug('args=%s' % args)
-if capture_output:
root_logger.debug('stdout=%s' % stdout)
root_logger.debug('stderr=%s' % stderr)
if p.returncode != 0 and raiseonerr:
-raise CalledProcessError(p.returncode, args)
+raise CalledProcessError(p.returncode, arg_string)
return (stdout, stderr, p.returncode)
--
1.7.11.7
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
