LVM has started including a pile of WARNING: lines in its output. These need to be removed before the return value lvm() is used. --- blivet/devicelibs/lvm.py | 41 ++++++++++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 15 deletions(-)
diff --git a/blivet/devicelibs/lvm.py b/blivet/devicelibs/lvm.py index 09a4956..c521248 100644 --- a/blivet/devicelibs/lvm.py +++ b/blivet/devicelibs/lvm.py @@ -216,13 +216,22 @@ def is_valid_thin_pool_chunk_size(size, discard=False): else: return (size % LVM_THINP_MIN_CHUNK_SIZE == 0)
+def strip_lvm_warnings(buf): + """ Strip out lines starting with WARNING: + + :param str buf: A string returned from lvm + :returns: A list of strings + :rtype: list + """ + return [l for l in buf.splitlines() if l and not l.strip().startswith("WARNING:")] + def lvm(args, capture=False, ignore_errors=False): """ Runs lvm with specified arguments.
:param bool capture: if True, return the output of the command. :param bool ignore_errors: if True, do not raise LVMError on failure - :returns: the output of the command or None - :rtype: str or NoneType + :returns: list of strings from the output of the command or None + :rtype: list or NoneType :raises: LVMError if command fails """ argv = ["lvm"] + args + _getConfigArgs(args) @@ -230,7 +239,7 @@ def lvm(args, capture=False, ignore_errors=False): if ret and not ignore_errors: raise LVMError("running "+ " ".join(argv) + " failed") if capture: - return out + return strip_lvm_warnings(out)
def pvcreate(device): # we force dataalignment=1024k since we cannot get lvm to tell us what @@ -324,9 +333,9 @@ def pvinfo(device=None): if device: args.append(device)
- buf = lvm(args, capture=True, ignore_errors=True) + lines = lvm(args, capture=True, ignore_errors=True) pvs = {} - for line in buf.splitlines(): + for line in lines: info = parse_lvm_vars(line) if len(info.keys()) != 11: log.warning("ignoring pvs output line: %s", line) @@ -421,8 +430,12 @@ def vginfo(vg_name): "-o", "uuid,size,free,extent_size,extent_count,free_count,pv_count", vg_name]
- buf = lvm(args, capture=True, ignore_errors=True) - info = parse_lvm_vars(buf) + lines = lvm(args, capture=True, ignore_errors=True) + try: + info = parse_lvm_vars(lines[0]) + except IndexError: + info = {} + if len(info.keys()) != 7: raise LVMError(_("vginfo failed for %s") % vg_name)
@@ -444,9 +457,9 @@ def lvs(vg_name=None): if vg_name: args.append(vg_name)
- buf = lvm(args, capture=True, ignore_errors=True) + lines = lvm(args, capture=True, ignore_errors=True) logvols = {} - for line in buf.splitlines(): + for line in lines: info = parse_lvm_vars(line) if len(info.keys()) != 6: log.debug("ignoring lvs output line: %s", line) @@ -461,10 +474,9 @@ def lvorigin(vg_name, lv_name): args = ["lvs", "--noheadings", "-o", "origin"] + \ ["%s/%s" % (vg_name, lv_name)]
- buf = lvm(args, capture=True, ignore_errors=True) - + lines = lvm(args, capture=True, ignore_errors=True) try: - origin = buf.splitlines()[0].strip() + origin = lines[0].strip() except IndexError: origin = ''
@@ -608,10 +620,9 @@ def thinlvpoolname(vg_name, lv_name): args = ["lvs", "--noheadings", "-o", "pool_lv"] + \ ["%s/%s" % (vg_name, lv_name)]
- buf = lvm(args, capture=True, ignore_errors=True) - + lines = lvm(args, capture=True, ignore_errors=True) try: - pool = buf.splitlines()[0].strip() + pool = lines[0].strip() except IndexError: pool = ''
----- Original Message -----
From: "Brian C. Lane" bcl@redhat.com To: anaconda-patches@lists.fedorahosted.org Sent: Monday, October 27, 2014 8:49:26 PM Subject: [master] Strip lvm WARNING: lines from output (#1157864)
LVM has started including a pile of WARNING: lines in its output. These need to be removed before the return value lvm() is used.
blivet/devicelibs/lvm.py | 41 ++++++++++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 15 deletions(-)
diff --git a/blivet/devicelibs/lvm.py b/blivet/devicelibs/lvm.py index 09a4956..c521248 100644 --- a/blivet/devicelibs/lvm.py +++ b/blivet/devicelibs/lvm.py @@ -216,13 +216,22 @@ def is_valid_thin_pool_chunk_size(size, discard=False): else: return (size % LVM_THINP_MIN_CHUNK_SIZE == 0)
+def strip_lvm_warnings(buf):
- """ Strip out lines starting with WARNING:
Strip out lvm warning lines.
- :param str buf: A string returned from lvm
- :returns: A list of strings
:returns: A list of strings, with warning lines stripped.
My reasoning for changes above is that the method is so simple and so little used that the only reason for making it a separate method is because we expect to have it grow as LVM develops sneakier and sneakier ways of inserting warnings into the output. So the comments might as well be generic in that expectation.
- :rtype: list
"list of str" seems better to me.
- """
- return [l for l in buf.splitlines() if l and not
l.strip().startswith("WARNING:")]
lstrip() (instead of strip()) makes slightly more sense here.
def lvm(args, capture=False, ignore_errors=False): """ Runs lvm with specified arguments.
:param bool capture: if True, return the output of the command. :param bool ignore_errors: if True, do not raise LVMError on failure
:returns: the output of the command or None:rtype: str or NoneType
:returns: list of strings from the output of the command or None:rtype: list or NoneType :raises: LVMError if command fails@@ -230,7 +239,7 @@ def lvm(args, capture=False, ignore_errors=False): if ret and not ignore_errors: raise LVMError("running "+ " ".join(argv) + " failed") if capture:
return out
return strip_lvm_warnings(out)def pvcreate(device): # we force dataalignment=1024k since we cannot get lvm to tell us what @@ -324,9 +333,9 @@ def pvinfo(device=None): if device: args.append(device)
- buf = lvm(args, capture=True, ignore_errors=True)
- lines = lvm(args, capture=True, ignore_errors=True) pvs = {}
- for line in buf.splitlines():
- for line in lines: info = parse_lvm_vars(line) if len(info.keys()) != 11: log.warning("ignoring pvs output line: %s", line)
@@ -421,8 +430,12 @@ def vginfo(vg_name): "-o", "uuid,size,free,extent_size,extent_count,free_count,pv_count", vg_name]
- buf = lvm(args, capture=True, ignore_errors=True)
- info = parse_lvm_vars(buf)
- lines = lvm(args, capture=True, ignore_errors=True)
- try:
info = parse_lvm_vars(lines[0])- except IndexError:
info = {}- if len(info.keys()) != 7: raise LVMError(_("vginfo failed for %s") % vg_name)
@@ -444,9 +457,9 @@ def lvs(vg_name=None): if vg_name: args.append(vg_name)
- buf = lvm(args, capture=True, ignore_errors=True)
- lines = lvm(args, capture=True, ignore_errors=True) logvols = {}
- for line in buf.splitlines():
- for line in lines: info = parse_lvm_vars(line) if len(info.keys()) != 6: log.debug("ignoring lvs output line: %s", line)
@@ -461,10 +474,9 @@ def lvorigin(vg_name, lv_name): args = ["lvs", "--noheadings", "-o", "origin"] + \ ["%s/%s" % (vg_name, lv_name)]
- buf = lvm(args, capture=True, ignore_errors=True)
- lines = lvm(args, capture=True, ignore_errors=True) try:
origin = buf.splitlines()[0].strip()
origin = lines[0].strip()@@ -608,10 +620,9 @@ def thinlvpoolname(vg_name, lv_name): args = ["lvs", "--noheadings", "-o", "pool_lv"] + \ ["%s/%s" % (vg_name, lv_name)]
- buf = lvm(args, capture=True, ignore_errors=True)
- lines = lvm(args, capture=True, ignore_errors=True) try:
pool = buf.splitlines()[0].strip()
pool = lines[0].strip()-- 1.9.3
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
Other than inline comments, ack.
- mulhern
On Tue, Oct 28, 2014 at 08:56:24AM -0400, Anne Mulhern wrote:
----- Original Message -----
From: "Brian C. Lane" bcl@redhat.com To: anaconda-patches@lists.fedorahosted.org Sent: Monday, October 27, 2014 8:49:26 PM Subject: [master] Strip lvm WARNING: lines from output (#1157864)
LVM has started including a pile of WARNING: lines in its output. These need to be removed before the return value lvm() is used.
blivet/devicelibs/lvm.py | 41 ++++++++++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 15 deletions(-)
diff --git a/blivet/devicelibs/lvm.py b/blivet/devicelibs/lvm.py index 09a4956..c521248 100644 --- a/blivet/devicelibs/lvm.py +++ b/blivet/devicelibs/lvm.py @@ -216,13 +216,22 @@ def is_valid_thin_pool_chunk_size(size, discard=False): else: return (size % LVM_THINP_MIN_CHUNK_SIZE == 0)
+def strip_lvm_warnings(buf):
- """ Strip out lines starting with WARNING:
Strip out lvm warning lines.
- :param str buf: A string returned from lvm
- :returns: A list of strings
:returns: A list of strings, with warning lines stripped.
My reasoning for changes above is that the method is so simple and so little used that the only reason for making it a separate method is because we expect to have it grow as LVM develops sneakier and sneakier ways of inserting warnings into the output. So the comments might as well be generic in that expectation.
- :rtype: list
"list of str" seems better to me.
- """
- return [l for l in buf.splitlines() if l and not
l.strip().startswith("WARNING:")]
lstrip() (instead of strip()) makes slightly more sense here.
Thanks, fixed all these locally.
On Mon, 2014-10-27 at 17:49 -0700, Brian C. Lane wrote:
LVM has started including a pile of WARNING: lines in its output. These need to be removed before the return value lvm() is used.
Isn't that a bug in the first place? I mean shouldn't these messages go to stderr and be ignored that way? Discussion about that is not a good strategy for f21-branch, but on master we should probably talk to the LVM team.
----- Original Message -----
From: "Vratislav Podzimek" vpodzime@redhat.com To: "anaconda patch review" anaconda-patches@lists.fedorahosted.org Sent: Wednesday, October 29, 2014 8:40:37 AM Subject: Re: [master] Strip lvm WARNING: lines from output (#1157864)
On Mon, 2014-10-27 at 17:49 -0700, Brian C. Lane wrote:
LVM has started including a pile of WARNING: lines in its output. These need to be removed before the return value lvm() is used.
Isn't that a bug in the first place? I mean shouldn't these messages go to stderr and be ignored that way? Discussion about that is not a good strategy for f21-branch, but on master we should probably talk to the LVM team.
-- Vratislav Podzimek
Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
Agreed.
- mulhern
On 10/29/2014 07:40 AM, Vratislav Podzimek wrote:
On Mon, 2014-10-27 at 17:49 -0700, Brian C. Lane wrote:
LVM has started including a pile of WARNING: lines in its output. These need to be removed before the return value lvm() is used.
Isn't that a bug in the first place? I mean shouldn't these messages go to stderr and be ignored that way? Discussion about that is not a good strategy for f21-branch, but on master we should probably talk to the LVM team.
blivet.util._run_program dupes stderr to stdout, so if you want to fix that it doesn't require the lvm team's involvement. I don't remember why we do that, but there may be some place in devicelibs we check the output for an error.
David
On Wed, 2014-10-29 at 08:44 -0500, David Lehman wrote:
On 10/29/2014 07:40 AM, Vratislav Podzimek wrote:
On Mon, 2014-10-27 at 17:49 -0700, Brian C. Lane wrote:
LVM has started including a pile of WARNING: lines in its output. These need to be removed before the return value lvm() is used.
Isn't that a bug in the first place? I mean shouldn't these messages go to stderr and be ignored that way? Discussion about that is not a good strategy for f21-branch, but on master we should probably talk to the LVM team.
blivet.util._run_program dupes stderr to stdout, so if you want to fix that it doesn't require the lvm team's involvement. I don't remember why we do that, but there may be some place in devicelibs we check the output for an error.
I didn't find anything like that. And I don't remember coming across stderr processing when rewriting most of the devicelibs/* code in libblockdev.
We only gather data from stdout and stderr contents may confuse our parsing code. But we may still be interested in the stderr so log it.
Signed-off-by: Vratislav Podzimek vpodzime@redhat.com --- blivet/util.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/blivet/util.py b/blivet/util.py index a69b694..01a4548 100644 --- a/blivet/util.py +++ b/blivet/util.py @@ -44,15 +44,21 @@ def _run_program(argv, root='/', stdin=None, env_prune=None): proc = subprocess.Popen(argv, stdin=stdin, stdout=subprocess.PIPE, - stderr=subprocess.STDOUT, + stderr=subprocess.PIPE, close_fds=True, preexec_fn=chroot, cwd=root, env=env)
- out = proc.communicate()[0] + out, err = proc.communicate() if out: + program_log.info("stdout:") for line in out.splitlines(): program_log.info("%s", line)
+ if err: + program_log.info("stderr:") + for line in err.splitlines(): + program_log.info("%s", line) + except OSError as e: program_log.error("Error running %s: %s", argv[0], e.strerror) raise
----- Original Message -----
From: "Vratislav Podzimek" vpodzime@redhat.com To: anaconda-patches@lists.fedorahosted.org Sent: Thursday, October 30, 2014 8:01:57 AM Subject: [blivet][master] Do not mix stdout and stderr when running utilities
We only gather data from stdout and stderr contents may confuse our parsing code. But we may still be interested in the stderr so log it.
Signed-off-by: Vratislav Podzimek vpodzime@redhat.com
blivet/util.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/blivet/util.py b/blivet/util.py index a69b694..01a4548 100644 --- a/blivet/util.py +++ b/blivet/util.py @@ -44,15 +44,21 @@ def _run_program(argv, root='/', stdin=None, env_prune=None): proc = subprocess.Popen(argv, stdin=stdin, stdout=subprocess.PIPE,
stderr=subprocess.STDOUT,
stderr=subprocess.PIPE, close_fds=True, preexec_fn=chroot, cwd=root, env=env)
out = proc.communicate()[0]
out, err = proc.communicate() if out:program_log.info("stdout:") for line in out.splitlines(): program_log.info("%s", line)if err:program_log.info("stderr:")for line in err.splitlines():program_log.info("%s", line)except OSError as e: program_log.error("Error running %s: %s", argv[0], e.strerror) raise-- 1.9.3
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
I do not like conflating standard out and standard error. So, generally speaking, I'm in favor of this change.
I also do not see any methods in devicelibs that seem to expect to read stderr from stdout. This doesn't mean there are none, but it's heartening.
I looked around outside devicelibs and found a number of calls to util.*capture_output* methods that were worth investigating. However, only fcoe.fcoe.addSan() seems to me to contain some calls which seem to expect that out will contain stderr.
This patch isn't really complete until at the least those calls have been investigated and appropriate action taken, however.
- mulhern
On Thu, 2014-10-30 at 09:51 -0400, Anne Mulhern wrote:
----- Original Message -----
From: "Vratislav Podzimek" vpodzime@redhat.com To: anaconda-patches@lists.fedorahosted.org Sent: Thursday, October 30, 2014 8:01:57 AM Subject: [blivet][master] Do not mix stdout and stderr when running utilities
We only gather data from stdout and stderr contents may confuse our parsing code. But we may still be interested in the stderr so log it.
Signed-off-by: Vratislav Podzimek vpodzime@redhat.com
blivet/util.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/blivet/util.py b/blivet/util.py index a69b694..01a4548 100644 --- a/blivet/util.py +++ b/blivet/util.py @@ -44,15 +44,21 @@ def _run_program(argv, root='/', stdin=None, env_prune=None): proc = subprocess.Popen(argv, stdin=stdin, stdout=subprocess.PIPE,
stderr=subprocess.STDOUT,
stderr=subprocess.PIPE, close_fds=True, preexec_fn=chroot, cwd=root, env=env)
out = proc.communicate()[0]
out, err = proc.communicate() if out:program_log.info("stdout:") for line in out.splitlines(): program_log.info("%s", line)if err:program_log.info("stderr:")for line in err.splitlines():program_log.info("%s", line)except OSError as e: program_log.error("Error running %s: %s", argv[0], e.strerror) raise-- 1.9.3
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
I do not like conflating standard out and standard error. So, generally speaking, I'm in favor of this change.
I also do not see any methods in devicelibs that seem to expect to read stderr from stdout. This doesn't mean there are none, but it's heartening.
I looked around outside devicelibs and found a number of calls to util.*capture_output* methods that were worth investigating. However, only fcoe.fcoe.addSan() seems to me to contain some calls which seem to expect that out will contain stderr.
Not that I have much chance to understand what's going on there, but I'll have a look at that call.
On Thu, 2014-10-30 at 09:51 -0400, Anne Mulhern wrote:
----- Original Message -----
From: "Vratislav Podzimek" vpodzime@redhat.com To: anaconda-patches@lists.fedorahosted.org Sent: Thursday, October 30, 2014 8:01:57 AM Subject: [blivet][master] Do not mix stdout and stderr when running utilities
We only gather data from stdout and stderr contents may confuse our parsing code. But we may still be interested in the stderr so log it.
Signed-off-by: Vratislav Podzimek vpodzime@redhat.com
blivet/util.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/blivet/util.py b/blivet/util.py index a69b694..01a4548 100644 --- a/blivet/util.py +++ b/blivet/util.py @@ -44,15 +44,21 @@ def _run_program(argv, root='/', stdin=None, env_prune=None): proc = subprocess.Popen(argv, stdin=stdin, stdout=subprocess.PIPE,
stderr=subprocess.STDOUT,
stderr=subprocess.PIPE, close_fds=True, preexec_fn=chroot, cwd=root, env=env)
out = proc.communicate()[0]
out, err = proc.communicate() if out:program_log.info("stdout:") for line in out.splitlines(): program_log.info("%s", line)if err:program_log.info("stderr:")for line in err.splitlines():program_log.info("%s", line)except OSError as e: program_log.error("Error running %s: %s", argv[0], e.strerror) raise-- 1.9.3
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
I do not like conflating standard out and standard error. So, generally speaking, I'm in favor of this change.
I also do not see any methods in devicelibs that seem to expect to read stderr from stdout. This doesn't mean there are none, but it's heartening.
I looked around outside devicelibs and found a number of calls to util.*capture_output* methods that were worth investigating. However, only fcoe.fcoe.addSan() seems to me to contain some calls which seem to expect that out will contain stderr.
This patch isn't really complete until at the least those calls have been investigated and appropriate action taken, however.
Okay so the addSan method returns an error message in case of failure which is constructed from the mangled stdout+stderr of 'fipvlan' invocation. Such message is then prepended with the string "Activating FCoE SAN failed:" and shown to the user by Anaconda. I can confirm that the 'fipvlan' utility really uses its stderr for error messages.
So we can: 1) drop this proposed patch and pretend that it is okay to mangle stdout and stderr 2) apply it as it is and live with the fact that users won't see the fipvlan's stderr output (we will, in the program.log) 3) add a 'err_to_out' parameter to run_program used only by addSan
Any suggestions? I think 3) is easy enough to be the best choice, but YMMV.
On 10/31/2014 07:44 AM, Vratislav Podzimek wrote:
On Thu, 2014-10-30 at 09:51 -0400, Anne Mulhern wrote:
----- Original Message -----
From: "Vratislav Podzimek" vpodzime@redhat.com To: anaconda-patches@lists.fedorahosted.org Sent: Thursday, October 30, 2014 8:01:57 AM Subject: [blivet][master] Do not mix stdout and stderr when running utilities
We only gather data from stdout and stderr contents may confuse our parsing code. But we may still be interested in the stderr so log it.
Signed-off-by: Vratislav Podzimek vpodzime@redhat.com
blivet/util.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/blivet/util.py b/blivet/util.py index a69b694..01a4548 100644 --- a/blivet/util.py +++ b/blivet/util.py @@ -44,15 +44,21 @@ def _run_program(argv, root='/', stdin=None, env_prune=None): proc = subprocess.Popen(argv, stdin=stdin, stdout=subprocess.PIPE,
stderr=subprocess.STDOUT,
stderr=subprocess.PIPE, close_fds=True, preexec_fn=chroot, cwd=root, env=env)
out = proc.communicate()[0]
out, err = proc.communicate() if out:program_log.info("stdout:") for line in out.splitlines(): program_log.info("%s", line)if err:program_log.info("stderr:")for line in err.splitlines():program_log.info("%s", line)except OSError as e: program_log.error("Error running %s: %s", argv[0], e.strerror) raise-- 1.9.3
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
I do not like conflating standard out and standard error. So, generally speaking, I'm in favor of this change.
I also do not see any methods in devicelibs that seem to expect to read stderr from stdout. This doesn't mean there are none, but it's heartening.
I looked around outside devicelibs and found a number of calls to util.*capture_output* methods that were worth investigating. However, only fcoe.fcoe.addSan() seems to me to contain some calls which seem to expect that out will contain stderr.
This patch isn't really complete until at the least those calls have been investigated and appropriate action taken, however.
Okay so the addSan method returns an error message in case of failure which is constructed from the mangled stdout+stderr of 'fipvlan' invocation. Such message is then prepended with the string "Activating FCoE SAN failed:" and shown to the user by Anaconda. I can confirm that the 'fipvlan' utility really uses its stderr for error messages.
So we can:
- drop this proposed patch and pretend that it is okay to mangle stdout
and stderr 2) apply it as it is and live with the fact that users won't see the fipvlan's stderr output (we will, in the program.log) 3) add a 'err_to_out' parameter to run_program used only by addSan
Any suggestions? I think 3) is easy enough to be the best choice, but YMMV.
Something along the lines of #3 sounds best, #2 being next best. Another option would be to add a third item to the return value of _run_program that contains the stderr output and another wrapper function called capture_output_with_errors or similar.
David
On Fri, Oct 31, 2014 at 01:44:02PM +0100, Vratislav Podzimek wrote:
So we can:
- drop this proposed patch and pretend that it is okay to mangle stdout
and stderr 2) apply it as it is and live with the fact that users won't see the fipvlan's stderr output (we will, in the program.log) 3) add a 'err_to_out' parameter to run_program used only by addSan
Any suggestions? I think 3) is easy enough to be the best choice, but YMMV.
Option #3 seems best to me. Callers should hopefully know when they would need to mix the two.
We only gather data from stdout and stderr contents may confuse our parsing code. However, we may still be interested in the stderr so log it and allow caller code to specify that stderr should go to stdout.
Signed-off-by: Vratislav Podzimek vpodzime@redhat.com --- blivet/fcoe.py | 4 ++-- blivet/util.py | 17 ++++++++++++++--- 2 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/blivet/fcoe.py b/blivet/fcoe.py index 707ec50..81c5c97 100644 --- a/blivet/fcoe.py +++ b/blivet/fcoe.py @@ -119,13 +119,13 @@ class fcoe(object): util.run_program(["dcbtool", "sc", nic, "app:fcoe", "e:1", "a:1", "w:1"]) rc, out = util.run_program_and_capture_output(["fipvlan", "-c", "-s", "-f", - "-fcoe", nic]) + "-fcoe", nic], stderr_to_stdout=True) else: if auto_vlan: # certain network configrations require the VLAN layer module: util.run_program(["modprobe", "8021q"]) rc, out = util.run_program_and_capture_output(["fipvlan", '-c', '-s', '-f', - "-fcoe", nic]) + "-fcoe", nic], stderr_to_stdout=True) else: f = open("/sys/module/libfcoe/parameters/create", "w") f.write(nic) diff --git a/blivet/util.py b/blivet/util.py index e3670a9..3e60e0b 100644 --- a/blivet/util.py +++ b/blivet/util.py @@ -23,7 +23,7 @@ from threading import Lock program_log_lock = Lock()
-def _run_program(argv, root='/', stdin=None, env_prune=None): +def _run_program(argv, root='/', stdin=None, env_prune=None, stderr_to_stdout=False): if env_prune is None: env_prune = []
@@ -40,19 +40,30 @@ def _run_program(argv, root='/', stdin=None, env_prune=None): for var in env_prune: env.pop(var, None)
+ if stderr_to_stdout: + stderr_dir = subprocess.STDOUT + else: + stderr_dir = subprocess.PIPE try: proc = subprocess.Popen(argv, stdin=stdin, stdout=subprocess.PIPE, - stderr=subprocess.STDOUT, + stderr=stderr_dir, close_fds=True, preexec_fn=chroot, cwd=root, env=env)
- out = proc.communicate()[0] + out, err = proc.communicate() if out: + if not stderr_to_stdout: + program_log.info("stdout:") for line in out.splitlines(): program_log.info("%s", line)
+ if not stderr_to_stdout and err: + program_log.info("stderr:") + for line in err.splitlines(): + program_log.info("%s", line) + except OSError as e: program_log.error("Error running %s: %s", argv[0], e.strerror) raise
On Thu, Jan 22, 2015 at 12:00:01PM +0100, Vratislav Podzimek wrote:
We only gather data from stdout and stderr contents may confuse our parsing code. However, we may still be interested in the stderr so log it and allow caller code to specify that stderr should go to stdout.
Big ack. Odd that we weren't doing that already.
----- Original Message -----
From: "Vratislav Podzimek" vpodzime@redhat.com To: anaconda-patches@lists.fedorahosted.org Sent: Thursday, January 22, 2015 6:00:01 AM Subject: [PATCHv2] Do not mix stdout and stderr when running utilities unless requested
We only gather data from stdout and stderr contents may confuse our parsing code. However, we may still be interested in the stderr so log it and allow caller code to specify that stderr should go to stdout.
Signed-off-by: Vratislav Podzimek vpodzime@redhat.com
blivet/fcoe.py | 4 ++-- blivet/util.py | 17 ++++++++++++++--- 2 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/blivet/fcoe.py b/blivet/fcoe.py index 707ec50..81c5c97 100644 --- a/blivet/fcoe.py +++ b/blivet/fcoe.py @@ -119,13 +119,13 @@ class fcoe(object): util.run_program(["dcbtool", "sc", nic, "app:fcoe", "e:1", "a:1", "w:1"]) rc, out = util.run_program_and_capture_output(["fipvlan", "-c", "-s", "-f",
"-fcoe", nic])
"-fcoe", nic],stderr_to_stdout=True) else: if auto_vlan: # certain network configrations require the VLAN layer module: util.run_program(["modprobe", "8021q"]) rc, out = util.run_program_and_capture_output(["fipvlan", '-c', '-s', '-f',
"-fcoe", nic])
"-fcoe", nic],stderr_to_stdout=True) else: f = open("/sys/module/libfcoe/parameters/create", "w") f.write(nic) diff --git a/blivet/util.py b/blivet/util.py index e3670a9..3e60e0b 100644 --- a/blivet/util.py +++ b/blivet/util.py @@ -23,7 +23,7 @@ from threading import Lock program_log_lock = Lock()
-def _run_program(argv, root='/', stdin=None, env_prune=None): +def _run_program(argv, root='/', stdin=None, env_prune=None, stderr_to_stdout=False): if env_prune is None: env_prune = []
@@ -40,19 +40,30 @@ def _run_program(argv, root='/', stdin=None, env_prune=None): for var in env_prune: env.pop(var, None)
if stderr_to_stdout:stderr_dir = subprocess.STDOUTelse:stderr_dir = subprocess.PIPE try: proc = subprocess.Popen(argv, stdin=stdin, stdout=subprocess.PIPE,
stderr=subprocess.STDOUT,
stderr=stderr_dir, close_fds=True, preexec_fn=chroot, cwd=root, env=env)
out = proc.communicate()[0]
out, err = proc.communicate() if out:if not stderr_to_stdout:program_log.info("stdout:") for line in out.splitlines(): program_log.info("%s", line)if not stderr_to_stdout and err:
Probably better to just have:
if err:
If subprocess.Popen is working, then err implies not stderr_to_stdout.
And if it isn't, we might as well still no what was printed to stderr.
program_log.info("stderr:")for line in err.splitlines():program_log.info("%s", line)except OSError as e: program_log.error("Error running %s: %s", argv[0], e.strerror) raise-- 2.1.0
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
- mulhern
On Thu, 2015-01-22 at 11:48 -0500, Anne Mulhern wrote:
----- Original Message -----
From: "Vratislav Podzimek" vpodzime@redhat.com To: anaconda-patches@lists.fedorahosted.org Sent: Thursday, January 22, 2015 6:00:01 AM Subject: [PATCHv2] Do not mix stdout and stderr when running utilities unless requested
We only gather data from stdout and stderr contents may confuse our parsing code. However, we may still be interested in the stderr so log it and allow caller code to specify that stderr should go to stdout.
Signed-off-by: Vratislav Podzimek vpodzime@redhat.com
blivet/fcoe.py | 4 ++-- blivet/util.py | 17 ++++++++++++++--- 2 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/blivet/fcoe.py b/blivet/fcoe.py index 707ec50..81c5c97 100644 --- a/blivet/fcoe.py +++ b/blivet/fcoe.py @@ -119,13 +119,13 @@ class fcoe(object): util.run_program(["dcbtool", "sc", nic, "app:fcoe", "e:1", "a:1", "w:1"]) rc, out = util.run_program_and_capture_output(["fipvlan", "-c", "-s", "-f",
"-fcoe", nic])
"-fcoe", nic],stderr_to_stdout=True) else: if auto_vlan: # certain network configrations require the VLAN layer module: util.run_program(["modprobe", "8021q"]) rc, out = util.run_program_and_capture_output(["fipvlan", '-c', '-s', '-f',
"-fcoe", nic])
"-fcoe", nic],stderr_to_stdout=True) else: f = open("/sys/module/libfcoe/parameters/create", "w") f.write(nic) diff --git a/blivet/util.py b/blivet/util.py index e3670a9..3e60e0b 100644 --- a/blivet/util.py +++ b/blivet/util.py @@ -23,7 +23,7 @@ from threading import Lock program_log_lock = Lock()
-def _run_program(argv, root='/', stdin=None, env_prune=None): +def _run_program(argv, root='/', stdin=None, env_prune=None, stderr_to_stdout=False): if env_prune is None: env_prune = []
@@ -40,19 +40,30 @@ def _run_program(argv, root='/', stdin=None, env_prune=None): for var in env_prune: env.pop(var, None)
if stderr_to_stdout:stderr_dir = subprocess.STDOUTelse:stderr_dir = subprocess.PIPE try: proc = subprocess.Popen(argv, stdin=stdin, stdout=subprocess.PIPE,
stderr=subprocess.STDOUT,
stderr=stderr_dir, close_fds=True, preexec_fn=chroot, cwd=root, env=env)
out = proc.communicate()[0]
out, err = proc.communicate() if out:if not stderr_to_stdout:program_log.info("stdout:") for line in out.splitlines(): program_log.info("%s", line)if not stderr_to_stdout and err:Probably better to just have:
if err:If subprocess.Popen is working, then err implies not stderr_to_stdout.
And if it isn't, we might as well still no what was printed to stderr.
If subprocess.Popen doesn't work as expected, we are doomed for many other reasons than this particular one so let's assume it works. I still like the check of the flag being done in a short-circuit way first. It just naturally fits in there first and if I were to make up some other reason, let's say that checking a bool flag is cheaper than checking the boolean representation of a string.
On 01/22/2015 05:00 AM, Vratislav Podzimek wrote:
We only gather data from stdout and stderr contents may confuse our parsing code. However, we may still be interested in the stderr so log it and allow caller code to specify that stderr should go to stdout.
Signed-off-by: Vratislav Podzimek vpodzime@redhat.com
blivet/fcoe.py | 4 ++-- blivet/util.py | 17 ++++++++++++++--- 2 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/blivet/fcoe.py b/blivet/fcoe.py index 707ec50..81c5c97 100644 --- a/blivet/fcoe.py +++ b/blivet/fcoe.py @@ -119,13 +119,13 @@ class fcoe(object): util.run_program(["dcbtool", "sc", nic, "app:fcoe", "e:1", "a:1", "w:1"]) rc, out = util.run_program_and_capture_output(["fipvlan", "-c", "-s", "-f",
"-fcoe", nic])
"-fcoe", nic], stderr_to_stdout=True) else: if auto_vlan: # certain network configrations require the VLAN layer module: util.run_program(["modprobe", "8021q"]) rc, out = util.run_program_and_capture_output(["fipvlan", '-c', '-s', '-f',
"-fcoe", nic])
"-fcoe", nic], stderr_to_stdout=True) else: f = open("/sys/module/libfcoe/parameters/create", "w") f.write(nic)diff --git a/blivet/util.py b/blivet/util.py index e3670a9..3e60e0b 100644 --- a/blivet/util.py +++ b/blivet/util.py @@ -23,7 +23,7 @@ from threading import Lock program_log_lock = Lock()
-def _run_program(argv, root='/', stdin=None, env_prune=None): +def _run_program(argv, root='/', stdin=None, env_prune=None, stderr_to_stdout=False): if env_prune is None: env_prune = []
@@ -40,19 +40,30 @@ def _run_program(argv, root='/', stdin=None, env_prune=None): for var in env_prune: env.pop(var, None)
if stderr_to_stdout:stderr_dir = subprocess.STDOUTelse:stderr_dir = subprocess.PIPE try: proc = subprocess.Popen(argv, stdin=stdin, stdout=subprocess.PIPE,
stderr=subprocess.STDOUT,
stderr=stderr_dir, close_fds=True, preexec_fn=chroot, cwd=root, env=env)
out = proc.communicate()[0]
out, err = proc.communicate() if out:if not stderr_to_stdout:program_log.info("stdout:") for line in out.splitlines(): program_log.info("%s", line)if not stderr_to_stdout and err:program_log.info("stderr:")for line in err.splitlines():program_log.info("%s", line)except OSError as e: program_log.error("Error running %s: %s", argv[0], e.strerror) raise
Looks good to me. Thanks.
David
On Wed, Oct 29, 2014 at 01:40:37PM +0100, Vratislav Podzimek wrote:
On Mon, 2014-10-27 at 17:49 -0700, Brian C. Lane wrote:
LVM has started including a pile of WARNING: lines in its output. These need to be removed before the return value lvm() is used.
Isn't that a bug in the first place? I mean shouldn't these messages go to stderr and be ignored that way? Discussion about that is not a good strategy for f21-branch, but on master we should probably talk to the LVM team.
Ends up it was both. I still think it's safer to leave this in place.
On Wed, 2014-10-29 at 18:19 -0700, Brian C. Lane wrote:
On Wed, Oct 29, 2014 at 01:40:37PM +0100, Vratislav Podzimek wrote:
On Mon, 2014-10-27 at 17:49 -0700, Brian C. Lane wrote:
LVM has started including a pile of WARNING: lines in its output. These need to be removed before the return value lvm() is used.
Isn't that a bug in the first place? I mean shouldn't these messages go to stderr and be ignored that way? Discussion about that is not a good strategy for f21-branch, but on master we should probably talk to the LVM team.
Ends up it was both. I still think it's safer to leave this in place.
It cannot hurt. I was more afraid of starting fixing such things this way. I'm looking at the stderr stuff right now and I hope we don't need strderr anywhere.
----- Original Message -----
From: "Brian C. Lane" bcl@redhat.com To: anaconda-patches@lists.fedorahosted.org Sent: Wednesday, October 29, 2014 9:19:11 PM Subject: Re: [master] Strip lvm WARNING: lines from output (#1157864)
On Wed, Oct 29, 2014 at 01:40:37PM +0100, Vratislav Podzimek wrote:
On Mon, 2014-10-27 at 17:49 -0700, Brian C. Lane wrote:
LVM has started including a pile of WARNING: lines in its output. These need to be removed before the return value lvm() is used.
Isn't that a bug in the first place? I mean shouldn't these messages go to stderr and be ignored that way? Discussion about that is not a good strategy for f21-branch, but on master we should probably talk to the LVM team.
Ends up it was both. I still think it's safer to leave this in place.
-- Brian C. Lane | Anaconda Team | IRC: bcl #anaconda | Port Orchard, WA (PST8PDT) _______________________________________________ anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
If those WARNING messages are actually going to stdout then we should file a bug.
- mulhern
anaconda-patches@lists.fedorahosted.org
-
Anne Mulhern -
Brian C. Lane -
David Lehman -
Vratislav Podzimek